Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
dwin-gharibi wants to merge 6 commits into
base: master
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c42ac59
fix(CubeOps/internal/server/server.go): adding key the login rate lim…
dwin-gharibi Aug 18, 2026
32fb3b2
test(CubeOps/internal/auth/ratelimit_xff_test.go): adding up the xff …
dwin-gharibi Aug 18, 2026
b072f52
fix(CubeOps/internal/auth/ratelimit.go): cap the failure map and ship…
dwin-gharibi Aug 18, 2026
92fb351
test(CubeOps/internal/auth/ratelimit_xff_test.go): exercise the sweep…
dwin-gharibi Aug 18, 2026
25c2b61
fix(deploy/kubernetes/chart/values.yaml): default to trusting no prox…
dwin-gharibi Aug 18, 2026
7e5a7d6
test(CubeOps/internal/config/config_trusted_proxies_test.go): cover t…
dwin-gharibi Aug 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 21 additions & 11 deletions CubeOps/internal/auth/ratelimit.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ type loginLimiter struct {
window time.Duration
}

const sweepThreshold = 4096

var defaultLoginLimiter = &loginLimiter{
failures: make(map[string][]time.Time),
limit: 5, // 5 failed attempts
Expand All @@ -41,6 +43,9 @@ func (l *loginLimiter) recordFailure(ip string) {
defer l.mu.Unlock()
now := time.Now()
cutoff := now.Add(-l.window)
if len(l.failures) >= sweepThreshold {

@cubesandboxbot cubesandboxbot Bot Aug 18, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

l.sweepLocked(cutoff)
}
fails := l.failures[ip]
// Drop expired entries.
kept := fails[:0]
Expand All @@ -57,6 +62,22 @@ func (l *loginLimiter) recordFailure(ip string) {
}
}

func (l *loginLimiter) sweepLocked(cutoff time.Time) {
for ip, fails := range l.failures {
kept := fails[:0]
for _, t := range fails {
if t.After(cutoff) {
kept = append(kept, t)
}
}
if len(kept) == 0 {
delete(l.failures, ip)
} else {
l.failures[ip] = kept
}
}
}

// isBlocked reports whether the IP has exceeded the failure limit.
// It also prunes expired entries and deletes the map entry when empty,
// so read-only checks also contribute to memory hygiene.
Expand All @@ -82,18 +103,7 @@ func (l *loginLimiter) isBlocked(ip string) bool {
return count >= l.limit
}

// clientIP extracts the client IP from the request, honoring
// X-Forwarded-For (set by nginx). Falls back to RemoteAddr.
func clientIP(c *gin.Context) string {
if xff := c.GetHeader("X-Forwarded-For"); xff != "" {
// Use the first (leftmost) address — that is the original client.
for i := 0; i < len(xff); i++ {
if xff[i] == ',' {
return xff[:i]
}
}
return xff
}
return c.ClientIP()
}

Expand Down
152 changes: 152 additions & 0 deletions CubeOps/internal/auth/ratelimit_xff_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,152 @@
// Copyright (c) 2026 Tencent Inc.
// SPDX-License-Identifier: Apache-2.0

package auth

import (
"net/http"
"net/http/httptest"
"testing"
"time"

"github.com/gin-gonic/gin"
)

func newLimiterRouter(t *testing.T, trustedProxies []string) *gin.Engine {
t.Helper()
gin.SetMode(gin.TestMode)
r := gin.New()
if err := r.SetTrustedProxies(trustedProxies); err != nil {
t.Fatalf("SetTrustedProxies: %v", err)
}
r.POST("/login", LoginRateLimit(), func(c *gin.Context) {
markLoginFailure(c)
c.Status(http.StatusUnauthorized)
})
return r
}

func attempt(r *gin.Engine, peer string, xff string) int {
req := httptest.NewRequest(http.MethodPost, "/login", nil)
req.RemoteAddr = peer
if xff != "" {
req.Header.Set("X-Forwarded-For", xff)
}
w := httptest.NewRecorder()
r.ServeHTTP(w, req)
return w.Code
}

func countBlocked(r *gin.Engine, peer string, xffs []string) int {
blocked := 0
for _, xff := range xffs {
if attempt(r, peer, xff) == http.StatusTooManyRequests {
blocked++
}
}
return blocked
}

func resetLimiter(t *testing.T) {
t.Helper()
defaultLoginLimiter.mu.Lock()
defaultLoginLimiter.failures = map[string][]time.Time{}
defaultLoginLimiter.mu.Unlock()
}

func TestRotatingForwardedForCannotBypassTheLimiter(t *testing.T) {
resetLimiter(t)
r := newLimiterRouter(t, nil)

xffs := make([]string, 0, 40)
for i := 0; i < 40; i++ {
xffs = append(xffs, "10.1.1."+string(rune('0'+i%10))+"9")
}

blocked := countBlocked(r, "203.0.113.9:1234", xffs)
if blocked == 0 {
t.Fatal("rotating X-Forwarded-For bypassed the limiter entirely")
}
if blocked < 30 {
t.Fatalf("only %d/40 attempts were blocked; the peer IP is not being used as the key", blocked)
}
}

func TestWhitespacePaddedForwardedForCannotBypassTheLimiter(t *testing.T) {
resetLimiter(t)
r := newLimiterRouter(t, nil)

xffs := make([]string, 0, 20)
for i := 0; i < 20; i++ {
pad := ""
for j := 0; j < i; j++ {
pad += " "
}
xffs = append(xffs, pad+"198.51.100.7")
}

if blocked := countBlocked(r, "203.0.113.9:1234", xffs); blocked == 0 {
t.Fatal("whitespace-padded X-Forwarded-For bypassed the limiter")
}
}

func TestLimiterStillFiresWithoutForwardedFor(t *testing.T) {
resetLimiter(t)
r := newLimiterRouter(t, nil)

blocked := 0
for i := 0; i < 10; i++ {
if attempt(r, "203.0.113.9:1234", "") == http.StatusTooManyRequests {
blocked++
}
}
if blocked == 0 {
t.Fatal("baseline broken: the limiter never fired")
}
}

func TestDistinctPeerIPsGetIndependentBuckets(t *testing.T) {
resetLimiter(t)
r := newLimiterRouter(t, nil)

for i := 0; i < 6; i++ {
attempt(r, "203.0.113.9:1234", "")
}
if code := attempt(r, "203.0.113.9:1234", ""); code != http.StatusTooManyRequests {
t.Fatalf("first peer should be blocked, got %d", code)
}
if code := attempt(r, "203.0.113.10:1234", ""); code == http.StatusTooManyRequests {
t.Fatal("a different peer IP was blocked by another peer's failures")
}
}

func TestTrustedProxyForwardedForIsHonoured(t *testing.T) {
resetLimiter(t)
r := newLimiterRouter(t, []string{"203.0.113.9"})

for i := 0; i < 6; i++ {
attempt(r, "203.0.113.9:1234", "198.51.100.7")
}
if code := attempt(r, "203.0.113.9:1234", "198.51.100.7"); code != http.StatusTooManyRequests {
t.Fatalf("a client behind a trusted proxy should be blocked, got %d", code)
}
if code := attempt(r, "203.0.113.9:1234", "198.51.100.8"); code == http.StatusTooManyRequests {
t.Fatal("a different client behind the same trusted proxy was blocked")
}
}

func TestSweepBoundsTheFailureMap(t *testing.T) {
l := &loginLimiter{failures: map[string][]time.Time{}, limit: 5, window: time.Millisecond}
for i := 0; i < sweepThreshold+50; i++ {
l.recordFailure("10.9." + string(rune('0'+i%10)) + "." + string(rune('0'+(i/10)%10)))

@cubesandboxbot cubesandboxbot Bot Aug 18, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
time.Sleep(5 * time.Millisecond)
l.recordFailure("10.9.9.9")

l.mu.Lock()
size := len(l.failures)
l.mu.Unlock()
if size > sweepThreshold {
t.Fatalf("failure map grew to %d entries, above the sweep threshold %d", size, sweepThreshold)
}
}
27 changes: 21 additions & 6 deletions CubeOps/internal/config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,12 +34,13 @@ import (
// Config holds all CubeOps runtime configuration.
type Config struct {
// Server
Bind string `yaml:"bind"`
LogLevel string `yaml:"log_level"`
LogDir string `yaml:"log_dir"`
LogFileNum int `yaml:"log_file_num"`
LogFileSize int `yaml:"log_file_size"`
JWTSecret string `yaml:"jwt_secret"`
Bind string `yaml:"bind"`
LogLevel string `yaml:"log_level"`
LogDir string `yaml:"log_dir"`
LogFileNum int `yaml:"log_file_num"`
LogFileSize int `yaml:"log_file_size"`
JWTSecret string `yaml:"jwt_secret"`
TrustedProxies []string `yaml:"trusted_proxies"`

// Database — either a single URL or the individual fields below.
DatabaseURL string `yaml:"database_url"`
Expand Down Expand Up @@ -281,6 +282,9 @@ func overrideFromEnv(cfg *Config) {
if v := os.Getenv("JWT_SECRET"); v != "" {
cfg.JWTSecret = v
}
if v := os.Getenv("CUBE_OPS_TRUSTED_PROXIES"); v != "" {
cfg.TrustedProxies = splitAndTrim(v)
}
if v := os.Getenv("DATABASE_URL"); v != "" {
cfg.DatabaseURL = v
}
Expand Down Expand Up @@ -325,3 +329,14 @@ func overrideFromEnv(cfg *Config) {
}
}
}

@cubesandboxbot cubesandboxbot Bot Aug 18, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


func splitAndTrim(v string) []string {
parts := strings.Split(v, ",")
out := make([]string, 0, len(parts))
for _, p := range parts {
if p = strings.TrimSpace(p); p != "" {
out = append(out, p)
}
}
return out
}
4 changes: 4 additions & 0 deletions CubeOps/internal/server/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,10 @@ func (s *Server) buildRouter() *gin.Engine {
// to stdout and bypasses any logger the operator has configured.
gin.SetMode(gin.ReleaseMode)
r := gin.New()

@cubesandboxbot cubesandboxbot Bot Aug 18, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if err := r.SetTrustedProxies(s.cfg.TrustedProxies); err != nil {

@cubesandboxbot cubesandboxbot Bot Aug 18, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

logging.G(context.Background()).Errorf("invalid trusted_proxies, falling back to trusting none: err=%q", err.Error())
_ = r.SetTrustedProxies(nil)
}
r.Use(requestLogger())
r.Use(cubeopsRecovery())

Expand Down
Loading