Tidy: Complete TrustedProxies feature (gin-gonic#2887)

(cherry picked from commit 3918132)

README.md

@@ -2130,11 +2130,17 @@ Gin lets you specify which headers to hold the real client IP (if any),
 as well as specifying which proxies (or direct clients) you trust to
 specify one of these headers.
 
-The `TrustedProxies` slice on your `gin.Engine` specifes network addresses or
-network CIDRs from where clients which their request headers related to client
+Use function `SetTrustedProxies()` on your `gin.Engine` to specify network addresses
+or network CIDRs from where clients which their request headers related to client
 IP can be trusted. They can be IPv4 addresses, IPv4 CIDRs, IPv6 addresses or
 IPv6 CIDRs.
 
+**Attention:** Gin trust all proxies by default if you don't specify a trusted 
+proxy using the function above, **this is NOT safe**. At the same time, if you don't
+use any proxy, you can disable this feature by using `Engine.SetTrustedProxies(nil)`,
+then `Context.ClientIP()` will return the remote address directly to avoid some
+unnecessary computation.
+
 ```go
 import (
 	"fmt"
@@ -2145,7 +2151,7 @@ import (
 func main() {
 
 	router := gin.Default()
-	router.TrustedProxies = []string{"192.168.1.2"}
+	router.SetTrustedProxies([]string{"192.168.1.2"})
 
 	router.GET("/", func(c *gin.Context) {
 		// If the client is 192.168.1.2, use the X-Forwarded-For

context.go

@@ -771,7 +771,7 @@ func (c *Context) ClientIP() string {
 // RemoteIP parses the IP from Request.RemoteAddr, normalizes and returns the IP (without the port).
 // It also checks if the remoteIP is a trusted proxy or not.
 // In order to perform this validation, it will see if the IP is contained within at least one of the CIDR blocks
-// defined in Engine.TrustedProxies
+// defined by Engine.SetTrustedProxies()
 func (c *Context) RemoteIP() (net.IP, bool) {
 	ip, _, err := net.SplitHostPort(strings.TrimSpace(c.Request.RemoteAddr))
 	if err != nil {

context_test.go

@@ -1415,6 +1415,10 @@ func TestContextClientIP(t *testing.T) {
 	c.engine.RemoteIPHeaders = []string{"X-Forwarded-For"}
 	assert.Equal(t, "40.40.40.40", c.ClientIP())
 
+	// Disabled TrustedProxies feature
+	_ = c.engine.SetTrustedProxies(nil)
+	assert.Equal(t, "40.40.40.40", c.ClientIP())
+
 	// Last proxy is trusted, but the RemoteAddr is not
 	_ = c.engine.SetTrustedProxies([]string{"30.30.30.30"})
 	assert.Equal(t, "40.40.40.40", c.ClientIP())

gin.go

@@ -11,6 +11,7 @@ import (
 	"net/http"
 	"os"
 	"path"
+	"reflect"
 	"strings"
 	"sync"
 
@@ -27,6 +28,8 @@ var (
 
 var defaultPlatform string
 
+var defaultTrustedCIDRs = []*net.IPNet{{IP: net.IP{0x0, 0x0, 0x0, 0x0}, Mask: net.IPMask{0x0, 0x0, 0x0, 0x0}}} // 0.0.0.0/0
+
 // HandlerFunc defines the handler used by gin middleware as return value.
 type HandlerFunc func(*Context)
 
@@ -119,15 +122,9 @@ type Engine struct {
 	// List of headers used to obtain the client IP when
 	// `(*gin.Engine).ForwardedByClientIP` is `true` and
 	// `(*gin.Context).Request.RemoteAddr` is matched by at least one of the
-	// network origins of `(*gin.Engine).TrustedProxies`.
+	// network origins of list defined by `(*gin.Engine).SetTrustedProxies()`.
 	RemoteIPHeaders []string
 
-	// List of network origins (IPv4 addresses, IPv4 CIDRs, IPv6 addresses or
-	// IPv6 CIDRs) from which to trust request's headers that contain
-	// alternative client IP when `(*gin.Engine).ForwardedByClientIP` is
-	// `true`.
-	TrustedProxies []string
-
 	// If set to a constant of value gin.Platform*, trusts the headers set by
 	// that platform, for example to determine the client IP
 	TrustedPlatform string
@@ -147,6 +144,7 @@ type Engine struct {
 	pool             sync.Pool
 	trees            methodTrees
 	maxParams        uint16
+	trustedProxies   []string
 	trustedCIDRs     []*net.IPNet
 }
 
@@ -174,7 +172,6 @@ func New() *Engine {
 		HandleMethodNotAllowed: false,
 		ForwardedByClientIP:    true,
 		RemoteIPHeaders:        []string{"X-Forwarded-For", "X-Real-IP"},
-		TrustedProxies:         []string{"0.0.0.0/0"},
 		TrustedPlatform:        defaultPlatform,
 		UseRawPath:             false,
 		RemoveExtraSlash:       false,
@@ -183,7 +180,8 @@ func New() *Engine {
 		trees:                  make(methodTrees, 0, 9),
 		delims:                 render.Delims{Left: "{{", Right: "}}"},
 		secureJSONPrefix:       "while(1);",
-		trustedCIDRs:           []*net.IPNet{{IP: net.IP{0x0, 0x0, 0x0, 0x0}, Mask: net.IPMask{0x0, 0x0, 0x0, 0x0}}},
+		trustedProxies:         []string{"0.0.0.0/0"},
+		trustedCIDRs:           defaultTrustedCIDRs,
 	}
 	engine.RouterGroup.engine = engine
 	engine.pool.New = func() interface{} {
@@ -342,9 +340,9 @@ func iterate(path, method string, routes RoutesInfo, root *node) RoutesInfo {
 func (engine *Engine) Run(addr ...string) (err error) {
 	defer func() { debugPrintError(err) }()
 
-	err = engine.parseTrustedProxies()
-	if err != nil {
-		return err
+	if engine.isUnsafeTrustedProxies() {
+		debugPrint("[WARNING] You trusted all proxies, this is NOT safe. We recommend you to set a value.\n" +
+			"Please check https://pkg.go.dev/github.com/gin-gonic/gin#readme-don-t-trust-all-proxies for details.")
 	}
 
 	address := resolveAddress(addr)
@@ -354,12 +352,12 @@ func (engine *Engine) Run(addr ...string) (err error) {
 }
 
 func (engine *Engine) prepareTrustedCIDRs() ([]*net.IPNet, error) {
-	if engine.TrustedProxies == nil {
+	if engine.trustedProxies == nil {
 		return nil, nil
 	}
 
-	cidr := make([]*net.IPNet, 0, len(engine.TrustedProxies))
-	for _, trustedProxy := range engine.TrustedProxies {
+	cidr := make([]*net.IPNet, 0, len(engine.trustedProxies))
+	for _, trustedProxy := range engine.trustedProxies {
 		if !strings.Contains(trustedProxy, "/") {
 			ip := parseIP(trustedProxy)
 			if ip == nil {
@@ -382,13 +380,25 @@ func (engine *Engine) prepareTrustedCIDRs() ([]*net.IPNet, error) {
 	return cidr, nil
 }
 
-// SetTrustedProxies  set Engine.TrustedProxies
+// SetTrustedProxies set a list of network origins (IPv4 addresses,
+// IPv4 CIDRs, IPv6 addresses or IPv6 CIDRs) from which to trust
+// request's headers that contain alternative client IP when
+// `(*gin.Engine).ForwardedByClientIP` is `true`. `TrustedProxies`
+// feature is enabled by default, and it also trusts all proxies
+// by default. If you want to disable this feature, use
+// Engine.SetTrustedProxies(nil), then Context.ClientIP() will
+// return the remote address directly.
 func (engine *Engine) SetTrustedProxies(trustedProxies []string) error {
-	engine.TrustedProxies = trustedProxies
+	engine.trustedProxies = trustedProxies
 	return engine.parseTrustedProxies()
 }
 
-// parseTrustedProxies parse Engine.TrustedProxies to Engine.trustedCIDRs
+// isUnsafeTrustedProxies compares Engine.trustedCIDRs and defaultTrustedCIDRs, it's not safe if equal (returns true)
+func (engine *Engine) isUnsafeTrustedProxies() bool {
+	return reflect.DeepEqual(engine.trustedCIDRs, defaultTrustedCIDRs)
+}
+
+// parseTrustedProxies parse Engine.trustedProxies to Engine.trustedCIDRs
 func (engine *Engine) parseTrustedProxies() error {
 	trustedCIDRs, err := engine.prepareTrustedCIDRs()
 	engine.trustedCIDRs = trustedCIDRs
@@ -416,9 +426,9 @@ func (engine *Engine) RunTLS(addr, certFile, keyFile string) (err error) {
 	debugPrint("Listening and serving HTTPS on %s\n", addr)
 	defer func() { debugPrintError(err) }()
 
-	err = engine.parseTrustedProxies()
-	if err != nil {
-		return err
+	if engine.isUnsafeTrustedProxies() {
+		debugPrint("[WARNING] You trusted all proxies, this is NOT safe. We recommend you to set a value.\n" +
+			"Please check https://pkg.go.dev/github.com/gin-gonic/gin#readme-don-t-trust-all-proxies for details.")
 	}
 
 	err = http.ListenAndServeTLS(addr, certFile, keyFile, engine)
@@ -432,9 +442,9 @@ func (engine *Engine) RunUnix(file string) (err error) {
 	debugPrint("Listening and serving HTTP on unix:/%s", file)
 	defer func() { debugPrintError(err) }()
 
-	err = engine.parseTrustedProxies()
-	if err != nil {
-		return err
+	if engine.isUnsafeTrustedProxies() {
+		debugPrint("[WARNING] You trusted all proxies, this is NOT safe. We recommend you to set a value.\n" +
+			"Please check https://pkg.go.dev/github.com/gin-gonic/gin#readme-don-t-trust-all-proxies for details.")
 	}
 
 	listener, err := net.Listen("unix", file)
@@ -455,9 +465,9 @@ func (engine *Engine) RunFd(fd int) (err error) {
 	debugPrint("Listening and serving HTTP on fd@%d", fd)
 	defer func() { debugPrintError(err) }()
 
-	err = engine.parseTrustedProxies()
-	if err != nil {
-		return err
+	if engine.isUnsafeTrustedProxies() {
+		debugPrint("[WARNING] You trusted all proxies, this is NOT safe. We recommend you to set a value.\n" +
+			"Please check https://pkg.go.dev/github.com/gin-gonic/gin#readme-don-t-trust-all-proxies for details.")
 	}
 
 	f := os.NewFile(uintptr(fd), fmt.Sprintf("fd@%d", fd))
@@ -476,9 +486,9 @@ func (engine *Engine) RunListener(listener net.Listener) (err error) {
 	debugPrint("Listening and serving HTTP on listener what's bind with address@%s", listener.Addr())
 	defer func() { debugPrintError(err) }()
 
-	err = engine.parseTrustedProxies()
-	if err != nil {
-		return err
+	if engine.isUnsafeTrustedProxies() {
+		debugPrint("[WARNING] You trusted all proxies, this is NOT safe. We recommend you to set a value.\n" +
+			"Please check https://pkg.go.dev/github.com/gin-gonic/gin#readme-don-t-trust-all-proxies for details.")
 	}
 
 	err = http.Serve(listener, engine)

gin_integration_test.go

@@ -76,6 +76,12 @@ func TestRunEmpty(t *testing.T) {
 	testRequest(t, "http://localhost:8080/example")
 }
 
+func TestBadTrustedCIDRs(t *testing.T) {
+	router := New()
+	assert.Error(t, router.SetTrustedProxies([]string{"hello/world"}))
+}
+
+/* legacy tests
 func TestBadTrustedCIDRsForRun(t *testing.T) {
 	os.Setenv("PORT", "")
 	router := New()
@@ -143,6 +149,7 @@ func TestBadTrustedCIDRsForRunTLS(t *testing.T) {
 	router.TrustedProxies = []string{"hello/world"}
 	assert.Error(t, router.RunTLS(":8080", "./testdata/certificate/cert.pem", "./testdata/certificate/key.pem"))
 }
+*/
 
 func TestRunTLS(t *testing.T) {
 	router := New()

gin_test.go

@@ -539,79 +539,64 @@ func TestPrepareTrustedCIRDsWith(t *testing.T) {
 	// valid ipv4 cidr
 	{
 		expectedTrustedCIDRs := []*net.IPNet{parseCIDR("0.0.0.0/0")}
-		r.TrustedProxies = []string{"0.0.0.0/0"}
-
-		trustedCIDRs, err := r.prepareTrustedCIDRs()
+		err := r.SetTrustedProxies([]string{"0.0.0.0/0"})
 
 		assert.NoError(t, err)
-		assert.Equal(t, expectedTrustedCIDRs, trustedCIDRs)
+		assert.Equal(t, expectedTrustedCIDRs, r.trustedCIDRs)
 	}
 
 	// invalid ipv4 cidr
 	{
-		r.TrustedProxies = []string{"192.168.1.33/33"}
-
-		_, err := r.prepareTrustedCIDRs()
+		err := r.SetTrustedProxies([]string{"192.168.1.33/33"})
 
 		assert.Error(t, err)
 	}
 
 	// valid ipv4 address
 	{
 		expectedTrustedCIDRs := []*net.IPNet{parseCIDR("192.168.1.33/32")}
-		r.TrustedProxies = []string{"192.168.1.33"}
 
-		trustedCIDRs, err := r.prepareTrustedCIDRs()
+		err := r.SetTrustedProxies([]string{"192.168.1.33"})
 
 		assert.NoError(t, err)
-		assert.Equal(t, expectedTrustedCIDRs, trustedCIDRs)
+		assert.Equal(t, expectedTrustedCIDRs, r.trustedCIDRs)
 	}
 
 	// invalid ipv4 address
 	{
-		r.TrustedProxies = []string{"192.168.1.256"}
-
-		_, err := r.prepareTrustedCIDRs()
+		err := r.SetTrustedProxies([]string{"192.168.1.256"})
 
 		assert.Error(t, err)
 	}
 
 	// valid ipv6 address
 	{
 		expectedTrustedCIDRs := []*net.IPNet{parseCIDR("2002:0000:0000:1234:abcd:ffff:c0a8:0101/128")}
-		r.TrustedProxies = []string{"2002:0000:0000:1234:abcd:ffff:c0a8:0101"}
-
-		trustedCIDRs, err := r.prepareTrustedCIDRs()
+		err := r.SetTrustedProxies([]string{"2002:0000:0000:1234:abcd:ffff:c0a8:0101"})
 
 		assert.NoError(t, err)
-		assert.Equal(t, expectedTrustedCIDRs, trustedCIDRs)
+		assert.Equal(t, expectedTrustedCIDRs, r.trustedCIDRs)
 	}
 
 	// invalid ipv6 address
 	{
-		r.TrustedProxies = []string{"gggg:0000:0000:1234:abcd:ffff:c0a8:0101"}
-
-		_, err := r.prepareTrustedCIDRs()
+		err := r.SetTrustedProxies([]string{"gggg:0000:0000:1234:abcd:ffff:c0a8:0101"})
 
 		assert.Error(t, err)
 	}
 
 	// valid ipv6 cidr
 	{
 		expectedTrustedCIDRs := []*net.IPNet{parseCIDR("::/0")}
-		r.TrustedProxies = []string{"::/0"}
-
-		trustedCIDRs, err := r.prepareTrustedCIDRs()
+		err := r.SetTrustedProxies([]string{"::/0"})
 
 		assert.NoError(t, err)
-		assert.Equal(t, expectedTrustedCIDRs, trustedCIDRs)
+		assert.Equal(t, expectedTrustedCIDRs, r.trustedCIDRs)
 	}
 
 	// invalid ipv6 cidr
 	{
-		r.TrustedProxies = []string{"gggg:0000:0000:1234:abcd:ffff:c0a8:0101/129"}
-
-		_, err := r.prepareTrustedCIDRs()
+		err := r.SetTrustedProxies([]string{"gggg:0000:0000:1234:abcd:ffff:c0a8:0101/129"})
 
 		assert.Error(t, err)
 	}
@@ -623,36 +608,32 @@ func TestPrepareTrustedCIRDsWith(t *testing.T) {
 			parseCIDR("192.168.0.0/16"),
 			parseCIDR("172.16.0.1/32"),
 		}
-		r.TrustedProxies = []string{
+		err := r.SetTrustedProxies([]string{
 			"::/0",
 			"192.168.0.0/16",
 			"172.16.0.1",
-		}
-
-		trustedCIDRs, err := r.prepareTrustedCIDRs()
+		})
 
 		assert.NoError(t, err)
-		assert.Equal(t, expectedTrustedCIDRs, trustedCIDRs)
+		assert.Equal(t, expectedTrustedCIDRs, r.trustedCIDRs)
 	}
 
 	// invalid combination
 	{
-		r.TrustedProxies = []string{
+		err := r.SetTrustedProxies([]string{
 			"::/0",
 			"192.168.0.0/16",
 			"172.16.0.256",
-		}
-		_, err := r.prepareTrustedCIDRs()
+		})
 
 		assert.Error(t, err)
 	}
 
 	// nil value
 	{
-		r.TrustedProxies = nil
-		trustedCIDRs, err := r.prepareTrustedCIDRs()
+		err := r.SetTrustedProxies(nil)
 
-		assert.Nil(t, trustedCIDRs)
+		assert.Nil(t, r.trustedCIDRs)
 		assert.Nil(t, err)
 	}