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) <noreply@anthropic.com>
This commit is contained in:
parent
42a402a5b4
commit
c4c63f11e7
8 changed files with 164 additions and 1 deletions
|
|
@ -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"
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
|
|
|
|||
|
|
@ -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"]
|
||||
|
|
|
|||
|
|
@ -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())
|
||||
|
||||
|
|
|
|||
77
src/frontend/apiserver/router/routers_test.go
Normal file
77
src/frontend/apiserver/router/routers_test.go
Normal file
|
|
@ -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)
|
||||
}
|
||||
}
|
||||
|
|
@ -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 {
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue