From 13323f56cbc3879fe70cc9678efecf8411e9e719 Mon Sep 17 00:00:00 2001 From: bolkedebruin Date: Thu, 30 Apr 2026 18:47:46 +0200 Subject: [PATCH] Honor X-Forwarded-For only from a trusted-proxy CIDR (#189) EnrichContext used to copy the first X-Forwarded-For entry into the request identity unconditionally. The resulting AttrClientIp drives client-IP comparisons later in the gateway-access flow, and a direct caller could set XFF to anything they liked. Add a small package-level allow-list: * InitTrustedProxies(cidrs) parses operator-supplied CIDRs once at startup. A bad CIDR is fatal, an empty list disables XFF entirely. * EnrichContext takes the client IP from r.RemoteAddr (host portion) and only swaps in the first X-Forwarded-For entry when r.RemoteAddr itself sits in a trusted-proxy CIDR. AttrProxies is set from the remaining XFF entries on the same condition. Wire Server.TrustedProxies through configuration.go to web. --- CHANGELOG.md | 6 ++ README.md | 6 ++ UPGRADING.md | 26 +++++++++ cmd/rdpgw/config/configuration.go | 5 ++ cmd/rdpgw/main.go | 2 + cmd/rdpgw/web/context.go | 83 +++++++++++++++++++++------- cmd/rdpgw/web/context_test.go | 92 +++++++++++++++++++++++++++++++ 7 files changed, 201 insertions(+), 19 deletions(-) create mode 100644 cmd/rdpgw/web/context_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index c66df0b..593e0a5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,11 @@ and the project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0. ### Changed +- `X-Forwarded-For` is now honored only when the request arrives from + a `Server.TrustedProxies` CIDR. The default `Server.TrustedProxies` + is empty, so by default the request's `RemoteAddr` (host portion) is + the source of `AttrClientIp`. See [UPGRADING.md](UPGRADING.md) if + your deployment relies on a fronting proxy stamping XFF. - `server.hostselection: any` now refuses destinations that resolve to loopback, RFC1918, link-local, IPv6 ULA, unspecified, or multicast addresses, and only forwards to ports in `Server.AllowedDestinationPorts` @@ -20,5 +25,6 @@ and the project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0. ### Added +- `Server.TrustedProxies` (`[]string`, CIDR, default empty). - `Server.AllowedDestinationPorts` (`[]int`, default `[3389]`). - `Server.AllowPrivateDestinations` (`bool`, default `false`). diff --git a/README.md b/README.md index 64c1f09..01b9bea 100644 --- a/README.md +++ b/README.md @@ -178,6 +178,12 @@ Server: # link-local / IPv6 ULA / unspecified / multicast destinations unless # this is true. Default false. Has no effect on the curated host modes. # AllowPrivateDestinations: false + # CIDR allow-list of upstream proxies whose X-Forwarded-For header is + # trusted when deriving the client IP. Empty (default) makes the + # gateway ignore X-Forwarded-For and use the request's RemoteAddr. + # Set this to the proxy/load-balancer subnet that fronts the gateway. + # TrustedProxies: + # - 10.0.0.0/8 # a random strings of at least 32 characters to secure cookies on the client # make sure to share this across the different pods SessionKey: thisisasessionkeyreplacethisjetzt diff --git a/UPGRADING.md b/UPGRADING.md index 50989c1..0c6c009 100644 --- a/UPGRADING.md +++ b/UPGRADING.md @@ -2,6 +2,32 @@ ## Unreleased +### `X-Forwarded-For` is no longer trusted by default + +Previously rdpgw read the first `X-Forwarded-For` entry into the +request identity unconditionally. The resulting client IP attribute is +later compared against the value embedded in the gateway access +cookie, so any caller reaching rdpgw directly could set +`X-Forwarded-For` to any value and steer that binding. + +After upgrading, `X-Forwarded-For` is honored only when the request +arrives from a `Server.TrustedProxies` CIDR. Otherwise the client IP +comes from `r.RemoteAddr`. The default `Server.TrustedProxies` is +empty, so by default `X-Forwarded-For` is ignored entirely. + +If your deployment fronts rdpgw with a reverse proxy or load balancer +on a known subnet, list it: + +```yaml +Server: + TrustedProxies: + - 10.0.0.0/8 # the proxy's egress subnet +``` + +If no proxy fronts rdpgw, leave `TrustedProxies` empty -- the +request's `RemoteAddr` is the right source for client identity in +that case. + ### `hostselection: any` now refuses non-routable destinations and non-RDP ports by default Previously, when `server.hostselection: any` was set, rdpgw forwarded diff --git a/cmd/rdpgw/config/configuration.go b/cmd/rdpgw/config/configuration.go index ed2831b..3019f2f 100644 --- a/cmd/rdpgw/config/configuration.go +++ b/cmd/rdpgw/config/configuration.go @@ -64,6 +64,11 @@ type ServerConfig struct { // forward to loopback, RFC1918, link-local, and IPv6 ULA targets. // Default false. AllowPrivateDestinations bool `koanf:"allowprivatedestinations"` + // TrustedProxies is the CIDR allow-list of upstream proxies whose + // X-Forwarded-For header is honored when deriving the client IP. + // Empty (the default) makes the gateway ignore X-Forwarded-For + // entirely and use r.RemoteAddr. + TrustedProxies []string `koanf:"trustedproxies"` } type KerberosConfig struct { diff --git a/cmd/rdpgw/main.go b/cmd/rdpgw/main.go index 598ea98..6cef497 100644 --- a/cmd/rdpgw/main.go +++ b/cmd/rdpgw/main.go @@ -101,6 +101,8 @@ func main() { conf.Server.MaxSessionLength, ) + web.InitTrustedProxies(conf.Server.TrustedProxies) + // configure web backend w := &web.Config{ QueryInfo: security.QueryInfo, diff --git a/cmd/rdpgw/web/context.go b/cmd/rdpgw/web/context.go index af8e2d3..e85a8b4 100644 --- a/cmd/rdpgw/web/context.go +++ b/cmd/rdpgw/web/context.go @@ -1,14 +1,55 @@ package web import ( - "github.com/bolkedebruin/rdpgw/cmd/rdpgw/identity" - "github.com/jcmturner/goidentity/v6" "log" "net" "net/http" "strings" + + "github.com/bolkedebruin/rdpgw/cmd/rdpgw/identity" + "github.com/jcmturner/goidentity/v6" ) +// trustedProxyNets is the CIDR allow-list of upstream proxies whose +// X-Forwarded-For header is honored. Empty (the default) means XFF is +// ignored entirely and the client IP is taken from r.RemoteAddr. +var trustedProxyNets []*net.IPNet + +// InitTrustedProxies parses the operator-supplied CIDRs once at startup. +// A bad CIDR is fatal; an empty list disables XFF-derived client-IP +// attribution. +func InitTrustedProxies(cidrs []string) { + nets := make([]*net.IPNet, 0, len(cidrs)) + for _, raw := range cidrs { + _, n, err := net.ParseCIDR(raw) + if err != nil { + log.Fatalf("trustedproxies: invalid CIDR %q: %s", raw, err) + } + nets = append(nets, n) + } + trustedProxyNets = nets +} + +func remoteIsTrustedProxy(remoteAddr string) bool { + if len(trustedProxyNets) == 0 { + return false + } + host, _, err := net.SplitHostPort(remoteAddr) + if err != nil { + host = remoteAddr + } + ip := net.ParseIP(host) + if ip == nil { + return false + } + for _, n := range trustedProxyNets { + if n.Contains(ip) { + return true + } + } + return false +} + func EnrichContext(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { id, err := GetSessionIdentity(r) @@ -28,26 +69,30 @@ func EnrichContext(next http.Handler) http.Handler { log.Printf("Identity SessionId: %s, UserName: %s: Authenticated: %t", id.SessionId(), id.UserName(), id.Authenticated()) - h := r.Header.Get("X-Forwarded-For") - if h != "" { - var proxies []string - ips := strings.Split(h, ",") - for i := range ips { - ips[i] = strings.TrimSpace(ips[i]) - } - clientIp := ips[0] - if len(ips) > 1 { - proxies = ips[1:] - } - id.SetAttribute(identity.AttrClientIp, clientIp) - id.SetAttribute(identity.AttrProxies, proxies) + id.SetAttribute(identity.AttrRemoteAddr, r.RemoteAddr) + + remoteHost, _, err := net.SplitHostPort(r.RemoteAddr) + if err != nil { + remoteHost = r.RemoteAddr } - id.SetAttribute(identity.AttrRemoteAddr, r.RemoteAddr) - if h == "" { - clientIp, _, _ := net.SplitHostPort(r.RemoteAddr) - id.SetAttribute(identity.AttrClientIp, clientIp) + clientIp := remoteHost + var proxies []string + if remoteIsTrustedProxy(r.RemoteAddr) { + if h := r.Header.Get("X-Forwarded-For"); h != "" { + ips := strings.Split(h, ",") + for i := range ips { + ips[i] = strings.TrimSpace(ips[i]) + } + clientIp = ips[0] + if len(ips) > 1 { + proxies = ips[1:] + } + } } + id.SetAttribute(identity.AttrClientIp, clientIp) + id.SetAttribute(identity.AttrProxies, proxies) + next.ServeHTTP(w, identity.AddToRequestCtx(id, r)) }) } diff --git a/cmd/rdpgw/web/context_test.go b/cmd/rdpgw/web/context_test.go new file mode 100644 index 0000000..1b6eb01 --- /dev/null +++ b/cmd/rdpgw/web/context_test.go @@ -0,0 +1,92 @@ +package web + +import ( + "net/http" + "net/http/httptest" + "testing" + + "github.com/bolkedebruin/rdpgw/cmd/rdpgw/identity" +) + +// TestEnrichContextXForwardedForRequiresTrustedProxy asserts that the +// X-Forwarded-For header is only honored when the request arrives from a +// configured trusted proxy. A direct caller can otherwise set XFF to any +// value, taking control of the AttrClientIp attribute that downstream +// session-binding logic compares against. +func TestEnrichContextXForwardedForRequiresTrustedProxy(t *testing.T) { + cases := []struct { + name string + trustedProxies []string + remoteAddr string + xff string + wantClientIp string + }{ + { + name: "untrusted remote with no XFF uses RemoteAddr", + trustedProxies: nil, + remoteAddr: "198.51.100.7:5678", + xff: "", + wantClientIp: "198.51.100.7", + }, + { + name: "untrusted remote with XFF still uses RemoteAddr", + trustedProxies: nil, + remoteAddr: "198.51.100.7:5678", + xff: "10.20.30.40", + wantClientIp: "198.51.100.7", + }, + { + name: "untrusted remote outside allow-list with XFF uses RemoteAddr", + trustedProxies: []string{"10.0.0.0/8"}, + remoteAddr: "198.51.100.7:5678", + xff: "10.20.30.40", + wantClientIp: "198.51.100.7", + }, + { + name: "trusted remote with XFF honors first XFF entry", + trustedProxies: []string{"10.0.0.0/8"}, + remoteAddr: "10.1.2.3:5678", + xff: "203.0.113.42, 10.99.0.1", + wantClientIp: "203.0.113.42", + }, + { + name: "trusted remote without XFF uses RemoteAddr", + trustedProxies: []string{"10.0.0.0/8"}, + remoteAddr: "10.1.2.3:5678", + xff: "", + wantClientIp: "10.1.2.3", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + InitTrustedProxies(tc.trustedProxies) + t.Cleanup(func() { InitTrustedProxies(nil) }) + + var captured identity.Identity + next := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + captured = identity.FromRequestCtx(r) + w.WriteHeader(http.StatusOK) + }) + h := EnrichContext(next) + + req := httptest.NewRequest("GET", "/", nil) + req.RemoteAddr = tc.remoteAddr + if tc.xff != "" { + req.Header.Set("X-Forwarded-For", tc.xff) + } + + rr := httptest.NewRecorder() + h.ServeHTTP(rr, req) + + if captured == nil { + t.Fatal("middleware did not store an identity in the request context") + } + got, _ := captured.GetAttribute(identity.AttrClientIp).(string) + if got != tc.wantClientIp { + t.Errorf("AttrClientIp = %q, want %q (remoteAddr=%q xff=%q trusted=%v)", + got, tc.wantClientIp, tc.remoteAddr, tc.xff, tc.trustedProxies) + } + }) + } +}