From c4c63f11e7b9da501b9fce3c3acd8e42a11d0a04 Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Sat, 10 Oct 2026 08:35:13 +0000 Subject: [PATCH] fix(apiserver): trust X-Forwarded-For only from configured proxies (bug-1948) Cause: the router used gin.Default() without SetTrustedProxies, and gin then trusts forwarding headers from every peer, so ClientIP() returned whatever X-Forwarded-For a client sent: the IP whitelist and the IP bound into JWTs could be spoofed. Fix: new app.yaml option security.trusted_proxies (IP or CIDR list, default empty = trust no proxy, validated at startup) passed to SetTrustedProxies in Router(); an invalid list falls back to trusting none. The nginx samples note that the proxy must be listed there. Tests: TestRouter_IgnoresForwardedForByDefault, TestRouter_HonoursForwardedForFromTrustedProxy, TestResolveTrustedProxies. Co-Authored-By: Claude Opus 5.5 (1M context) --- src/frontend/apiserver/config/app.yaml | 7 +- src/frontend/apiserver/config/nginx.http.conf | 2 + .../apiserver/config/nginx.https.conf | 2 + .../apiserver/config/schema/app_schema.json | 5 ++ src/frontend/apiserver/router/routers.go | 8 ++ src/frontend/apiserver/router/routers_test.go | 77 +++++++++++++++++++ src/frontend/apiserver/utils/config.go | 30 ++++++++ src/frontend/apiserver/utils/config_test.go | 34 ++++++++ 8 files changed, 164 insertions(+), 1 deletion(-) create mode 100644 src/frontend/apiserver/router/routers_test.go diff --git a/src/frontend/apiserver/config/app.yaml b/src/frontend/apiserver/config/app.yaml index c986c34b..ae3d54c1 100644 --- a/src/frontend/apiserver/config/app.yaml +++ b/src/frontend/apiserver/config/app.yaml @@ -10,7 +10,12 @@ security: port: 8443 cert_file: "config/ssl/server.crt" key_file: "config/ssl/server.key" - + # Reverse proxies (IP or CIDR) whose X-Forwarded-For / X-Real-IP headers + # set the client IP used by the IP whitelist and the JWT IP binding. + # Empty (default): trust no proxy, the client IP is the TCP peer address. + # Behind nginx/HAProxy, list the proxy's address here, e.g. ["127.0.0.1"]. + trusted_proxies: [] + authentication: token: type: "jwt" diff --git a/src/frontend/apiserver/config/nginx.http.conf b/src/frontend/apiserver/config/nginx.http.conf index 0b90ad10..eff85dcb 100644 --- a/src/frontend/apiserver/config/nginx.http.conf +++ b/src/frontend/apiserver/config/nginx.http.conf @@ -44,6 +44,8 @@ http { location / { proxy_pass http://apiserver_backend; + # apiserver honours these headers only from addresses listed in + # app.yaml security.trusted_proxies: add this proxy's address there. proxy_set_header X-Real-IP $remote_addr; proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for; proxy_next_upstream error timeout http_500 http_502 http_503 http_504; diff --git a/src/frontend/apiserver/config/nginx.https.conf b/src/frontend/apiserver/config/nginx.https.conf index 67a00715..47b58fb9 100644 --- a/src/frontend/apiserver/config/nginx.https.conf +++ b/src/frontend/apiserver/config/nginx.https.conf @@ -51,6 +51,8 @@ http { location / { proxy_pass https://apiserver_backend; + # apiserver honours these headers only from addresses listed in + # app.yaml security.trusted_proxies: add this proxy's address there. proxy_set_header X-Real-IP $remote_addr; proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for; proxy_next_upstream error timeout http_500 http_502 http_503 http_504; diff --git a/src/frontend/apiserver/config/schema/app_schema.json b/src/frontend/apiserver/config/schema/app_schema.json index a07a4f53..ab399043 100644 --- a/src/frontend/apiserver/config/schema/app_schema.json +++ b/src/frontend/apiserver/config/schema/app_schema.json @@ -25,6 +25,11 @@ "key_file": { "type": "string" } }, "required": ["enabled", "port", "cert_file", "key_file"] + }, + "trusted_proxies": { + "type": "array", + "items": { "type": "string" }, + "description": "Reverse proxies (IP or CIDR) whose X-Forwarded-For/X-Real-IP set the client IP; empty trusts none" } }, "required": ["https"] diff --git a/src/frontend/apiserver/router/routers.go b/src/frontend/apiserver/router/routers.go index b0e68ef1..b2aae5ab 100644 --- a/src/frontend/apiserver/router/routers.go +++ b/src/frontend/apiserver/router/routers.go @@ -18,6 +18,14 @@ func Router() *gin.Engine { logger.Debug("Enter Router() function") r := gin.Default() + // Honour X-Forwarded-For / X-Real-IP only from configured reverse proxies + // (security.trusted_proxies, default none), so a client cannot choose the + // IP seen by the IP whitelist and the JWT IP binding (bug-1948). + if err := r.SetTrustedProxies(utils.TrustedProxiesYamlConfig); err != nil { + logger.Error("Invalid security.trusted_proxies, trusting no proxy", zap.Error(err)) + _ = r.SetTrustedProxies(nil) + } + // check access URL whitelist r.Use(middleware.WhitelistURLMiddleware()) diff --git a/src/frontend/apiserver/router/routers_test.go b/src/frontend/apiserver/router/routers_test.go new file mode 100644 index 00000000..3a0322cc --- /dev/null +++ b/src/frontend/apiserver/router/routers_test.go @@ -0,0 +1,77 @@ +package router + +import ( + "apiserver/utils" + "net/http" + "net/http/httptest" + "testing" + + "github.com/gin-gonic/gin" + "go.uber.org/zap" +) + +// setRouterTestConfig installs the minimal config Router() reads, with an IP +// whitelist of 10.0.0.0/8, and restores the previous values afterwards. +func setRouterTestConfig(t *testing.T) { + t.Helper() + prevLog, prevTop := utils.Log, utils.Global_TopDir + prevWL, prevRL, prevCORS := utils.WhitelistYamlConfig, utils.RateLimitYamlConfig, utils.CORSYamlConfig + t.Cleanup(func() { + utils.Log, utils.Global_TopDir = prevLog, prevTop + utils.WhitelistYamlConfig, utils.RateLimitYamlConfig, utils.CORSYamlConfig = prevWL, prevRL, prevCORS + }) + + gin.SetMode(gin.TestMode) + utils.Log = zap.NewNop() + utils.Global_TopDir = t.TempDir() + utils.WhitelistYamlConfig = &utils.WhitelistConfig{Whitelist: []string{"10.0.0.0/8"}} + utils.RateLimitYamlConfig = &utils.RateLimitConfig{ + Global: utils.EndpointLimit{Rate: 1000, Burst: 2000, Window: "second"}, + Endpoints: map[string]utils.EndpointLimit{}, + } + utils.CORSYamlConfig = nil +} + +func pingFrom(r *gin.Engine, remoteAddr, forwardedFor string) int { + req := httptest.NewRequest(http.MethodGet, "/ping", nil) + req.RemoteAddr = remoteAddr + if forwardedFor != "" { + req.Header.Set("X-Forwarded-For", forwardedFor) + } + w := httptest.NewRecorder() + r.ServeHTTP(w, req) + return w.Code +} + +// bug-1948: with no trusted proxies configured, X-Forwarded-For must not +// change the client IP seen by the IP whitelist (or the JWT IP binding). +func TestRouter_IgnoresForwardedForByDefault(t *testing.T) { + setRouterTestConfig(t) + r := Router() + + if got := pingFrom(r, "10.1.1.1:4000", ""); got != http.StatusOK { + t.Fatalf("whitelisted peer: status = %d, want 200", got) + } + if got := pingFrom(r, "192.0.2.10:4000", "10.1.1.1"); got != http.StatusForbidden { + t.Errorf("non-whitelisted peer spoofing X-Forwarded-For: status = %d, want 403", got) + } +} + +// bug-1948: X-Forwarded-For is honoured only from a configured trusted proxy. +func TestRouter_HonoursForwardedForFromTrustedProxy(t *testing.T) { + setRouterTestConfig(t) + prev := utils.TrustedProxiesYamlConfig + t.Cleanup(func() { utils.TrustedProxiesYamlConfig = prev }) + utils.TrustedProxiesYamlConfig = []string{"192.0.2.1"} + r := Router() + + if got := pingFrom(r, "192.0.2.1:4000", "10.1.1.1"); got != http.StatusOK { + t.Errorf("trusted proxy forwarding a whitelisted client: status = %d, want 200", got) + } + if got := pingFrom(r, "192.0.2.1:4000", "198.51.100.7"); got != http.StatusForbidden { + t.Errorf("trusted proxy forwarding a non-whitelisted client: status = %d, want 403", got) + } + if got := pingFrom(r, "192.0.2.99:4000", "10.1.1.1"); got != http.StatusForbidden { + t.Errorf("untrusted peer spoofing X-Forwarded-For: status = %d, want 403", got) + } +} diff --git a/src/frontend/apiserver/utils/config.go b/src/frontend/apiserver/utils/config.go index 260bfa2a..2420723f 100644 --- a/src/frontend/apiserver/utils/config.go +++ b/src/frontend/apiserver/utils/config.go @@ -4,6 +4,7 @@ import ( "errors" "fmt" "log" + "net" "os" "runtime" "strconv" @@ -56,6 +57,10 @@ type Authentication struct { type Security struct { HTTPS HTTPSConfig `yaml:"https"` + // TrustedProxies lists the reverse proxies (IP or CIDR) whose + // X-Forwarded-For / X-Real-IP headers set the client IP. Empty (the + // default) trusts no proxy: the client IP is the TCP peer address. + TrustedProxies []string `yaml:"trusted_proxies"` } type HTTPSConfig struct { Enabled bool `yaml:"enabled"` @@ -105,6 +110,7 @@ var ( AppYamlConfig *AppConfig AuthenticationYamlConfig *Authentication HttpsYamlConfig *HTTPSConfig + TrustedProxiesYamlConfig []string RateLimitYamlConfig *RateLimitConfig WhitelistYamlConfig *WhitelistConfig FileUploadYamlConfig *FileUploadConfig @@ -172,6 +178,11 @@ func InitConf() error { CertFile: viper.GetString("security.https.cert_file"), KeyFile: viper.GetString("security.https.key_file"), } + trustedProxies, err := resolveTrustedProxies(viper.GetStringSlice("security.trusted_proxies")) + if err != nil { + return err + } + TrustedProxiesYamlConfig = trustedProxies // parse rate limit section RateLimitYamlConfig = &RateLimitConfig{ @@ -310,6 +321,25 @@ func resolveMaxArraySize(v int) (effective int, clamped bool, err error) { } } +// resolveTrustedProxies validates security.trusted_proxies: every entry must +// be an IP address or a CIDR. An empty list means no proxy is trusted. +func resolveTrustedProxies(entries []string) ([]string, error) { + proxies := make([]string, 0, len(entries)) + for _, e := range entries { + e = strings.TrimSpace(e) + if e == "" { + continue + } + if net.ParseIP(e) == nil { + if _, _, err := net.ParseCIDR(e); err != nil { + return nil, fmt.Errorf("invalid security.trusted_proxies entry %q: must be an IP address or CIDR", e) + } + } + proxies = append(proxies, e) + } + return proxies, nil +} + // resolveJWTSecret returns the JWT secret from XCE_JWT_SECRET env var if set and non-empty, // otherwise falls back to the value from config file. func resolveJWTSecret(configValue string) string { diff --git a/src/frontend/apiserver/utils/config_test.go b/src/frontend/apiserver/utils/config_test.go index 5fb36a15..fd23919f 100644 --- a/src/frontend/apiserver/utils/config_test.go +++ b/src/frontend/apiserver/utils/config_test.go @@ -40,6 +40,40 @@ func TestResolveJWTSecret_EmptyEnvUsesConfig(t *testing.T) { } } +// bug-1948: security.trusted_proxies accepts IPs and CIDRs only; empty +// means no trusted proxy. +func TestResolveTrustedProxies(t *testing.T) { + cases := []struct { + name string + in []string + want []string + wantErr bool + }{ + {"unset trusts none", nil, []string{}, false}, + {"IP and CIDR", []string{"127.0.0.1", " 10.0.0.0/8 ", "::1"}, []string{"127.0.0.1", "10.0.0.0/8", "::1"}, false}, + {"blank entries dropped", []string{""}, []string{}, false}, + {"hostname refused", []string{"proxy.example.com"}, nil, true}, + {"bad CIDR refused", []string{"10.0.0.0/33"}, nil, true}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got, err := resolveTrustedProxies(tc.in) + if tc.wantErr { + if err == nil || !strings.Contains(err.Error(), "security.trusted_proxies") { + t.Fatalf("expected a security.trusted_proxies error, got %v (%v)", err, got) + } + return + } + if err != nil { + t.Fatalf("unexpected error %v", err) + } + if strings.Join(got, ",") != strings.Join(tc.want, ",") || got == nil { + t.Errorf("resolveTrustedProxies(%q) = %q, want %q", tc.in, got, tc.want) + } + }) + } +} + func TestResolveMaxArraySize(t *testing.T) { cases := []struct { name string