diff --git a/pkg/probod/probod.go b/pkg/probod/probod.go index 9fcd8d3cf..48d5b5328 100644 --- a/pkg/probod/probod.go +++ b/pkg/probod/probod.go @@ -778,8 +778,12 @@ func (impl *Implm) runApiServer( ctx, span := tracer.Start(ctx, "probod.runApiServer") defer span.End() - trustedProxies := parseIPs(impl.cfg.Api.ProxyProtocol.TrustedProxies) - handler = trustedproxy.NewMiddleware(trustedProxies)(handler) + trustedProxyMiddleware, err := trustedproxy.NewMiddleware(impl.cfg.Api.ProxyProtocol.TrustedProxies) + if err != nil { + span.RecordError(err) + return fmt.Errorf("cannot build trusted proxy middleware: %w", err) + } + handler = trustedProxyMiddleware(handler) apiServer := httpserver.NewServer( impl.cfg.Api.Addr, @@ -799,7 +803,11 @@ func (impl *Implm) runApiServer( } if len(impl.cfg.Api.ProxyProtocol.TrustedProxies) > 0 { - policy := proxyproto.TrustProxyHeaderFrom(parseIPs(impl.cfg.Api.ProxyProtocol.TrustedProxies)...) + policy, err := proxyproto.ConnStrictWhiteListPolicy(impl.cfg.Api.ProxyProtocol.TrustedProxies) + if err != nil { + span.RecordError(err) + return fmt.Errorf("cannot build proxy protocol policy: %w", err) + } listener = &proxyproto.Listener{ Listener: listener, @@ -972,7 +980,10 @@ func (impl *Implm) runTrustCenterServer( defer func() { _ = listener.Close() }() if len(impl.cfg.TrustCenter.ProxyProtocol.TrustedProxies) > 0 { - policy := proxyproto.TrustProxyHeaderFrom(parseIPs(impl.cfg.TrustCenter.ProxyProtocol.TrustedProxies)...) + policy, err := proxyproto.ConnStrictWhiteListPolicy(impl.cfg.TrustCenter.ProxyProtocol.TrustedProxies) + if err != nil { + return fmt.Errorf("cannot build proxy protocol policy: %w", err) + } listener = &proxyproto.Listener{ Listener: listener, @@ -1057,7 +1068,10 @@ func (impl *Implm) runTrustCenterServer( defer func() { _ = listener.Close() }() if len(impl.cfg.TrustCenter.ProxyProtocol.TrustedProxies) > 0 { - policy := proxyproto.TrustProxyHeaderFrom(parseIPs(impl.cfg.TrustCenter.ProxyProtocol.TrustedProxies)...) + policy, err := proxyproto.ConnStrictWhiteListPolicy(impl.cfg.TrustCenter.ProxyProtocol.TrustedProxies) + if err != nil { + return fmt.Errorf("cannot build proxy protocol policy: %w", err) + } listener = &proxyproto.Listener{ Listener: listener, @@ -1109,18 +1123,6 @@ func (impl *Implm) runTrustCenterServer( return ctx.Err() } -// parseIPs converts a slice of string IP addresses to net.IP. -// Invalid IPs are skipped. -func parseIPs(strs []string) []net.IP { - ips := make([]net.IP, 0, len(strs)) - for _, s := range strs { - if ip := net.ParseIP(s); ip != nil { - ips = append(ips, ip) - } - } - return ips -} - func oauth2ServerOptions(cfg OAuth2ServerConfig) []oauth2server.Option { var opts []oauth2server.Option diff --git a/pkg/server/trustedproxy/trustedproxy.go b/pkg/server/trustedproxy/trustedproxy.go index 1d3cf4bc8..3fe2b0f88 100644 --- a/pkg/server/trustedproxy/trustedproxy.go +++ b/pkg/server/trustedproxy/trustedproxy.go @@ -15,8 +15,10 @@ package trustedproxy import ( + "fmt" "net" "net/http" + "strings" ) var forwardedHeaders = []string{ @@ -26,22 +28,52 @@ var forwardedHeaders = []string{ // NewMiddleware returns an HTTP middleware that strips forwarded // headers from requests that did not originate from one of the given -// trusted proxy IPs. When the list is empty every request is treated -// as untrusted and the headers are always removed. -func NewMiddleware(trusted []net.IP) func(http.Handler) http.Handler { +// trusted proxies. Each entry in trusted may be either a single IP +// address (e.g. "10.0.0.1") or a CIDR range (e.g. "10.0.0.0/24"). +// When the list is empty every request is treated as untrusted and +// the headers are always removed. An error is returned if any entry +// is neither a valid IP nor a valid CIDR. +func NewMiddleware(trusted []string) (func(http.Handler) http.Handler, error) { + ips, nets, err := parseTrusted(trusted) + if err != nil { + return nil, err + } + return func(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if !isTrusted(r.RemoteAddr, trusted) { + if !isTrusted(r.RemoteAddr, ips, nets) { for _, h := range forwardedHeaders { r.Header.Del(h) } } next.ServeHTTP(w, r) }) - } + }, nil } -func isTrusted(remoteAddr string, trusted []net.IP) bool { +func parseTrusted(trusted []string) ([]net.IP, []*net.IPNet, error) { + ips := make([]net.IP, 0, len(trusted)) + nets := make([]*net.IPNet, 0, len(trusted)) + for _, entry := range trusted { + if strings.Contains(entry, "/") { + _, ipNet, err := net.ParseCIDR(entry) + if err != nil { + return nil, nil, fmt.Errorf("cannot parse CIDR %q: %w", entry, err) + } + nets = append(nets, ipNet) + continue + } + + ip := net.ParseIP(entry) + if ip == nil { + return nil, nil, fmt.Errorf("cannot parse IP address %q", entry) + } + ips = append(ips, ip) + } + return ips, nets, nil +} + +func isTrusted(remoteAddr string, ips []net.IP, nets []*net.IPNet) bool { host, _, err := net.SplitHostPort(remoteAddr) if err != nil { host = remoteAddr @@ -52,11 +84,17 @@ func isTrusted(remoteAddr string, trusted []net.IP) bool { return false } - for _, t := range trusted { + for _, t := range ips { if t.Equal(ip) { return true } } + for _, n := range nets { + if n.Contains(ip) { + return true + } + } + return false } diff --git a/pkg/server/trustedproxy/trustedproxy_test.go b/pkg/server/trustedproxy/trustedproxy_test.go index 9b831d3bf..42f914ed4 100644 --- a/pkg/server/trustedproxy/trustedproxy_test.go +++ b/pkg/server/trustedproxy/trustedproxy_test.go @@ -15,7 +15,6 @@ package trustedproxy_test import ( - "net" "net/http" "net/http/httptest" "testing" @@ -25,113 +24,159 @@ import ( "go.probo.inc/probo/pkg/server/trustedproxy" ) -func newRequest(remoteAddr string, headers map[string]string) *http.Request { - r := httptest.NewRequest(http.MethodGet, "/", nil) - r.RemoteAddr = remoteAddr +func runMiddleware(t *testing.T, trusted []string, remoteAddr string, headers map[string]string) *http.Request { + t.Helper() + + middleware, err := trustedproxy.NewMiddleware(trusted) + require.NoError(t, err) + + var captured *http.Request + handler := middleware(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + captured = r + })) + + req := httptest.NewRequest(http.MethodGet, "/", nil) + req.RemoteAddr = remoteAddr for k, v := range headers { - r.Header.Set(k, v) + req.Header.Set(k, v) } - return r + handler.ServeHTTP(httptest.NewRecorder(), req) + + require.NotNil(t, captured) + return captured } -func TestNewMiddleware(t *testing.T) { +func TestNewMiddleware_HeaderHandling(t *testing.T) { t.Parallel() - t.Run( - "strips forwarded headers from untrusted proxy", - func(t *testing.T) { - t.Parallel() - - trusted := []net.IP{net.ParseIP("10.0.0.1")} - middleware := trustedproxy.NewMiddleware(trusted) - - var captured *http.Request - handler := middleware(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - captured = r - })) - - r := newRequest("192.168.1.1:1234", map[string]string{ - "X-Forwarded-For": "203.0.113.50", - "Forwarded": "for=198.51.100.17", - }) - handler.ServeHTTP(httptest.NewRecorder(), r) - - require.NotNil(t, captured) - assert.Empty(t, captured.Header.Get("X-Forwarded-For")) - assert.Empty(t, captured.Header.Get("Forwarded")) + tests := []struct { + name string + trusted []string + remoteAddr string + expectPreserved bool + forwardedHeaders map[string]string + }{ + { + name: "untrusted proxy strips forwarded headers", + trusted: []string{"10.0.0.1"}, + remoteAddr: "192.168.1.1:1234", + expectPreserved: false, }, + { + name: "trusted proxy preserves forwarded headers", + trusted: []string{"10.0.0.1"}, + remoteAddr: "10.0.0.1:1234", + expectPreserved: true, + }, + { + name: "empty trusted list strips all forwarded headers", + trusted: nil, + remoteAddr: "10.0.0.1:1234", + expectPreserved: false, + }, + { + name: "multiple trusted IPs", + trusted: []string{"10.0.0.1", "10.0.0.2"}, + remoteAddr: "10.0.0.2:5678", + expectPreserved: true, + }, + { + name: "CIDR range trusts addresses within the range", + trusted: []string{"10.0.0.0/24"}, + remoteAddr: "10.0.0.50:1234", + expectPreserved: true, + }, + { + name: "CIDR range strips addresses outside the range", + trusted: []string{"10.0.0.0/24"}, + remoteAddr: "10.0.1.50:1234", + expectPreserved: false, + }, + { + name: "mixed IP and CIDR list trusts plain IP", + trusted: []string{"192.168.1.1", "10.0.0.0/24"}, + remoteAddr: "192.168.1.1:1234", + expectPreserved: true, + }, + { + name: "mixed IP and CIDR list trusts address in CIDR", + trusted: []string{"192.168.1.1", "10.0.0.0/24"}, + remoteAddr: "10.0.0.99:1234", + expectPreserved: true, + }, + { + name: "IPv6 CIDR range trusts addresses within the range", + trusted: []string{"fd00::/8"}, + remoteAddr: "[fd12:3456::1]:1234", + expectPreserved: true, + }, + { + name: "IPv6 CIDR range strips addresses outside the range", + trusted: []string{"fd00::/8"}, + remoteAddr: "[2001:db8::1]:1234", + expectPreserved: false, + }, + } + + const ( + xff = "203.0.113.50" + fwd = "for=198.51.100.17" ) - t.Run( - "preserves forwarded headers from trusted proxy", - func(t *testing.T) { - t.Parallel() + for _, tc := range tests { + t.Run( + tc.name, + func(t *testing.T) { + t.Parallel() - trusted := []net.IP{net.ParseIP("10.0.0.1")} - middleware := trustedproxy.NewMiddleware(trusted) + captured := runMiddleware( + t, + tc.trusted, + tc.remoteAddr, + map[string]string{ + "X-Forwarded-For": xff, + "Forwarded": fwd, + }, + ) - var captured *http.Request - handler := middleware(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - captured = r - })) - - r := newRequest("10.0.0.1:1234", map[string]string{ - "X-Forwarded-For": "203.0.113.50", - "Forwarded": "for=198.51.100.17", - }) - handler.ServeHTTP(httptest.NewRecorder(), r) - - require.NotNil(t, captured) - assert.Equal(t, "203.0.113.50", captured.Header.Get("X-Forwarded-For")) - assert.Equal(t, "for=198.51.100.17", captured.Header.Get("Forwarded")) - }, - ) - - t.Run( - "empty trusted list strips all forwarded headers", - func(t *testing.T) { - t.Parallel() - - middleware := trustedproxy.NewMiddleware(nil) - - var captured *http.Request - handler := middleware(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - captured = r - })) - - r := newRequest("10.0.0.1:1234", map[string]string{ - "X-Forwarded-For": "203.0.113.50", - }) - handler.ServeHTTP(httptest.NewRecorder(), r) - - require.NotNil(t, captured) - assert.Empty(t, captured.Header.Get("X-Forwarded-For")) - }, - ) - - t.Run( - "multiple trusted proxies", - func(t *testing.T) { - t.Parallel() - - trusted := []net.IP{ - net.ParseIP("10.0.0.1"), - net.ParseIP("10.0.0.2"), - } - middleware := trustedproxy.NewMiddleware(trusted) - - var captured *http.Request - handler := middleware(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - captured = r - })) - - r := newRequest("10.0.0.2:5678", map[string]string{ - "X-Forwarded-For": "203.0.113.50", - }) - handler.ServeHTTP(httptest.NewRecorder(), r) - - require.NotNil(t, captured) - assert.Equal(t, "203.0.113.50", captured.Header.Get("X-Forwarded-For")) - }, - ) + if tc.expectPreserved { + assert.Equal(t, xff, captured.Header.Get("X-Forwarded-For")) + assert.Equal(t, fwd, captured.Header.Get("Forwarded")) + } else { + assert.Empty(t, captured.Header.Get("X-Forwarded-For")) + assert.Empty(t, captured.Header.Get("Forwarded")) + } + }, + ) + } +} + +func TestNewMiddleware_InvalidInput(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + trusted []string + }{ + { + name: "invalid IP", + trusted: []string{"not-an-ip"}, + }, + { + name: "invalid CIDR mask", + trusted: []string{"10.0.0.0/99"}, + }, + } + + for _, tc := range tests { + t.Run( + tc.name, + func(t *testing.T) { + t.Parallel() + + _, err := trustedproxy.NewMiddleware(tc.trusted) + require.Error(t, err) + }, + ) + } }