From 4d41a2f01012ba016373dc5951592671d5e53659 Mon Sep 17 00:00:00 2001 From: Notealot <714804968@qq.com> Date: Wed, 29 Sep 2021 22:56:28 +0800 Subject: [PATCH 01/10] Perfect TrustedProxies feature --- README.md | 12 +++++-- context.go | 2 +- context_test.go | 4 +++ gin.go | 70 +++++++++++++++++++++++------------------ gin_integration_test.go | 7 +++++ gin_test.go | 57 +++++++++++---------------------- 6 files changed, 80 insertions(+), 72 deletions(-) diff --git a/README.md b/README.md index 10be5e155d..a24a8b6fa6 100644 --- a/README.md +++ b/README.md @@ -2202,11 +2202,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 specifies 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 this function, **this is not safe**. At the same time, if you don't use +any proxy, you can disable this feature by use `Engine.SetTrustedProxies(nil)`, +then `Context.ClientIP()` will return the remote address directly to avoid some +unnecessary computation. + ```go import ( "fmt" @@ -2217,7 +2223,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 diff --git a/context.go b/context.go index 370f608634..d211098b72 100644 --- a/context.go +++ b/context.go @@ -778,7 +778,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 { diff --git a/context_test.go b/context_test.go index ad16fbb1c6..07a35f523b 100644 --- a/context_test.go +++ b/context_test.go @@ -1409,6 +1409,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()) diff --git a/gin.go b/gin.go index 02a1062c31..153d0157ec 100644 --- a/gin.go +++ b/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 set 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 equals Engine.trustedCIDRs and defaultTrustedCIDRs, it's not safe if 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) diff --git a/gin_integration_test.go b/gin_integration_test.go index 094c46e871..0b67b542de 100644 --- a/gin_integration_test.go +++ b/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() diff --git a/gin_test.go b/gin_test.go index 678d95f2c8..21c43d1520 100644 --- a/gin_test.go +++ b/gin_test.go @@ -539,19 +539,15 @@ 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) } @@ -559,19 +555,16 @@ func TestPrepareTrustedCIRDsWith(t *testing.T) { // 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) } @@ -579,19 +572,15 @@ func TestPrepareTrustedCIRDsWith(t *testing.T) { // 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) } @@ -599,19 +588,15 @@ func TestPrepareTrustedCIRDsWith(t *testing.T) { // 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) } From 7d84c45579bfb1fc00cd70d86deedf401bf99b01 Mon Sep 17 00:00:00 2001 From: Notealot <714804968@qq.com> Date: Thu, 30 Sep 2021 12:06:53 +0800 Subject: [PATCH 02/10] some typo fix --- README.md | 4 ++-- gin.go | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index a24a8b6fa6..8c833c7124 100644 --- a/README.md +++ b/README.md @@ -2208,8 +2208,8 @@ 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 this function, **this is not safe**. At the same time, if you don't use -any proxy, you can disable this feature by use `Engine.SetTrustedProxies(nil)`, +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. diff --git a/gin.go b/gin.go index 153d0157ec..8083f89330 100644 --- a/gin.go +++ b/gin.go @@ -122,7 +122,7 @@ 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 list set by `(*gin.Engine).SetTrustedProxies()`. + // network origins of list defined by `(*gin.Engine).SetTrustedProxies()`. RemoteIPHeaders []string // If set to a constant of value gin.Platform*, trusts the headers set by @@ -393,7 +393,7 @@ func (engine *Engine) SetTrustedProxies(trustedProxies []string) error { return engine.parseTrustedProxies() } -// isUnsafeTrustedProxies equals Engine.trustedCIDRs and defaultTrustedCIDRs, it's not safe if true +// 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) } From 5b8833e76d15a71cc95a073fd749f51eb3e61255 Mon Sep 17 00:00:00 2001 From: Notealot <714804968@qq.com> Date: Fri, 1 Oct 2021 22:28:51 +0800 Subject: [PATCH 03/10] fix some typo --- README.md | 2 +- gin.go | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index 8c833c7124..62094ff101 100644 --- a/README.md +++ b/README.md @@ -2202,7 +2202,7 @@ 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. -Use function `SetTrustedProxies()` on your `gin.Engine` to specifies network addresses +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. diff --git a/gin.go b/gin.go index 8083f89330..af83161b68 100644 --- a/gin.go +++ b/gin.go @@ -341,7 +341,7 @@ func (engine *Engine) Run(addr ...string) (err error) { defer func() { debugPrintError(err) }() if engine.isUnsafeTrustedProxies() { - debugPrint("[WARNING] You trusted all proxies, this is not safe. We recommend you to set a value.\n" + + 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.") } @@ -427,7 +427,7 @@ func (engine *Engine) RunTLS(addr, certFile, keyFile string) (err error) { defer func() { debugPrintError(err) }() if engine.isUnsafeTrustedProxies() { - debugPrint("[WARNING] You trusted all proxies, this is not safe. We recommend you to set a value.\n" + + 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.") } @@ -443,7 +443,7 @@ func (engine *Engine) RunUnix(file string) (err error) { defer func() { debugPrintError(err) }() if engine.isUnsafeTrustedProxies() { - debugPrint("[WARNING] You trusted all proxies, this is not safe. We recommend you to set a value.\n" + + 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.") } @@ -466,7 +466,7 @@ func (engine *Engine) RunFd(fd int) (err error) { defer func() { debugPrintError(err) }() if engine.isUnsafeTrustedProxies() { - debugPrint("[WARNING] You trusted all proxies, this is not safe. We recommend you to set a value.\n" + + 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.") } @@ -487,7 +487,7 @@ func (engine *Engine) RunListener(listener net.Listener) (err error) { defer func() { debugPrintError(err) }() if engine.isUnsafeTrustedProxies() { - debugPrint("[WARNING] You trusted all proxies, this is not safe. We recommend you to set a value.\n" + + 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.") } From 6deb70c16c505ca6fe9b113e6810b2bbe0e72eda Mon Sep 17 00:00:00 2001 From: Notealot <714804968@qq.com> Date: Tue, 23 Nov 2021 10:45:35 +0800 Subject: [PATCH 04/10] readme hash link fix test --- README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index cad746d62c..444db3bc9c 100644 --- a/README.md +++ b/README.md @@ -78,7 +78,7 @@ Gin is a web framework written in Go (Golang). It features a martini-like API wi - [http2 server push](#http2-server-push) - [Define format for the log of routes](#define-format-for-the-log-of-routes) - [Set and get a cookie](#set-and-get-a-cookie) - - [Don't trust all proxies](#don't-trust-all-proxies) + - [Don't trust all proxies](#don't-trust-all-proxies) - [Testing](#testing) - [Users](#users) @@ -2197,7 +2197,7 @@ func main() { } ``` -## Don't trust all proxies +## Don't trust all proxies 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 From 68ed8eda7d295207a1b7cc84028e1e353aa465b4 Mon Sep 17 00:00:00 2001 From: Notealot <714804968@qq.com> Date: Tue, 23 Nov 2021 10:51:39 +0800 Subject: [PATCH 05/10] Revert "readme hash link fix test" This reverts commit 6deb70c16c505ca6fe9b113e6810b2bbe0e72eda. --- README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 444db3bc9c..cad746d62c 100644 --- a/README.md +++ b/README.md @@ -78,7 +78,7 @@ Gin is a web framework written in Go (Golang). It features a martini-like API wi - [http2 server push](#http2-server-push) - [Define format for the log of routes](#define-format-for-the-log-of-routes) - [Set and get a cookie](#set-and-get-a-cookie) - - [Don't trust all proxies](#don't-trust-all-proxies) + - [Don't trust all proxies](#don't-trust-all-proxies) - [Testing](#testing) - [Users](#users) @@ -2197,7 +2197,7 @@ func main() { } ``` -## Don't trust all proxies +## Don't trust all proxies 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 From 0b56a6001211eb81f6e9f0e7b78b3601d4914db1 Mon Sep 17 00:00:00 2001 From: Notealot <714804968@qq.com> Date: Mon, 29 Nov 2021 11:14:02 +0800 Subject: [PATCH 06/10] Refactor and fix --- README.md | 4 ++-- context.go | 52 ++++++++----------------------------------------- context_test.go | 10 +++++++++- gin.go | 41 +++++++++++++++++++++++++++++++++++++- 4 files changed, 59 insertions(+), 48 deletions(-) diff --git a/README.md b/README.md index cad746d62c..203fe79de7 100644 --- a/README.md +++ b/README.md @@ -78,7 +78,7 @@ Gin is a web framework written in Go (Golang). It features a martini-like API wi - [http2 server push](#http2-server-push) - [Define format for the log of routes](#define-format-for-the-log-of-routes) - [Set and get a cookie](#set-and-get-a-cookie) - - [Don't trust all proxies](#don't-trust-all-proxies) + - [Don't trust all proxies](#do-not-trust-all-proxies) - [Testing](#testing) - [Users](#users) @@ -2197,7 +2197,7 @@ func main() { } ``` -## Don't trust all proxies +## Do not trust all proxies 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 diff --git a/context.go b/context.go index 2aa91e11e0..8146be3758 100644 --- a/context.go +++ b/context.go @@ -757,10 +757,14 @@ func (c *Context) ClientIP() string { } } - remoteIP, trusted := c.RemoteIP() + // 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 by Engine.SetTrustedProxies() + remoteIP := net.ParseIP(c.RemoteIP()) if remoteIP == nil { return "" } + trusted := c.engine.isTrustedProxy(remoteIP) if trusted && c.engine.ForwardedByClientIP && c.engine.RemoteIPHeaders != nil { for _, headerName := range c.engine.RemoteIPHeaders { @@ -773,53 +777,13 @@ func (c *Context) ClientIP() string { return remoteIP.String() } -func (e *Engine) isTrustedProxy(ip net.IP) bool { - if e.trustedCIDRs != nil { - for _, cidr := range e.trustedCIDRs { - if cidr.Contains(ip) { - return true - } - } - } - return false -} - // 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 by Engine.SetTrustedProxies() -func (c *Context) RemoteIP() (net.IP, bool) { +func (c *Context) RemoteIP() string { ip, _, err := net.SplitHostPort(strings.TrimSpace(c.Request.RemoteAddr)) if err != nil { - return nil, false - } - remoteIP := net.ParseIP(ip) - if remoteIP == nil { - return nil, false - } - - return remoteIP, c.engine.isTrustedProxy(remoteIP) -} - -func (e *Engine) validateHeader(header string) (clientIP string, valid bool) { - if header == "" { - return "", false - } - items := strings.Split(header, ",") - for i := len(items) - 1; i >= 0; i-- { - ipStr := strings.TrimSpace(items[i]) - ip := net.ParseIP(ipStr) - if ip == nil { - return "", false - } - - // X-Forwarded-For is appended by proxy - // Check IPs in reverse order and stop when find untrusted proxy - if (i == 0) || (!e.isTrustedProxy(ip)) { - return ipStr, true - } + return "" } - return + return ip } // ContentType returns the Content-Type header of the request. diff --git a/context_test.go b/context_test.go index c286c0f4cb..4d002c2375 100644 --- a/context_test.go +++ b/context_test.go @@ -12,6 +12,7 @@ import ( "html/template" "io" "mime/multipart" + "net" "net/http" "net/http/httptest" "os" @@ -1404,6 +1405,11 @@ func TestContextClientIP(t *testing.T) { // Tests exercising the TrustedProxies functionality resetContextForClientIPTests(c) + // IPv6 support + c.Request.RemoteAddr = "[::1]:12345" + assert.Equal(t, "20.20.20.20", c.ClientIP()) + + resetContextForClientIPTests(c) // No trusted proxies _ = c.engine.SetTrustedProxies([]string{}) c.engine.RemoteIPHeaders = []string{"X-Forwarded-For"} @@ -1500,6 +1506,7 @@ func resetContextForClientIPTests(c *Context) { c.Request.Header.Set("CF-Connecting-IP", "60.60.60.60") c.Request.RemoteAddr = " 40.40.40.40:42123 " c.engine.TrustedPlatform = "" + c.engine.trustedCIDRs = defaultTrustedCIDRs c.engine.AppEngine = false } @@ -2051,7 +2058,8 @@ func TestRemoteIPFail(t *testing.T) { c, _ := CreateTestContext(httptest.NewRecorder()) c.Request, _ = http.NewRequest("POST", "/", nil) c.Request.RemoteAddr = "[:::]:80" - ip, trust := c.RemoteIP() + ip := net.ParseIP(c.RemoteIP()) + trust := c.engine.isTrustedProxy(ip) assert.Nil(t, ip) assert.False(t, trust) } diff --git a/gin.go b/gin.go index 1d9c9fee44..42f281f3c1 100644 --- a/gin.go +++ b/gin.go @@ -28,7 +28,13 @@ 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 +var defaultTrustedCIDRs = []*net.IPNet{ + {IP: net.IP{0x0, 0x0, 0x0, 0x0}, Mask: net.IPMask{0x0, 0x0, 0x0, 0x0}}, // 0.0.0.0/0 + { // ::/0 + IP: net.IP{0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0}, + Mask: net.IPMask{0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0}, + }, +} // HandlerFunc defines the handler used by gin middleware as return value. type HandlerFunc func(*Context) @@ -411,6 +417,39 @@ func (engine *Engine) parseTrustedProxies() error { return err } +// isTrustedProxy will check whether the IP address is included in the trusted list according to Engine.trustedCIDRs +func (engine *Engine) isTrustedProxy(ip net.IP) bool { + if engine.trustedCIDRs != nil { + for _, cidr := range engine.trustedCIDRs { + if cidr.Contains(ip) { + return true + } + } + } + return false +} + +// validateHeader will parse X-Forwarded-For header and return the trusted client IP address +func (engine *Engine) validateHeader(header string) (clientIP string, valid bool) { + if header != "" { + items := strings.Split(header, ",") + for i := len(items) - 1; i >= 0; i-- { + ipStr := strings.TrimSpace(items[i]) + ip := net.ParseIP(ipStr) + if ip == nil { + break + } + + // X-Forwarded-For is appended by proxy + // Check IPs in reverse order and stop when find untrusted proxy + if (i == 0) || (!engine.isTrustedProxy(ip)) { + return ipStr, true + } + } + } + return "", false +} + // parseIP parse a string representation of an IP and returns a net.IP with the // minimum byte representation or nil if input is invalid. func parseIP(ip string) net.IP { From 6196556531adb92be87ed845c115c435c712e445 Mon Sep 17 00:00:00 2001 From: Notealot <714804968@qq.com> Date: Mon, 29 Nov 2021 11:28:01 +0800 Subject: [PATCH 07/10] revert README.md changes --- README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 203fe79de7..e3d6ab2f84 100644 --- a/README.md +++ b/README.md @@ -78,7 +78,7 @@ Gin is a web framework written in Go (Golang). It features a martini-like API wi - [http2 server push](#http2-server-push) - [Define format for the log of routes](#define-format-for-the-log-of-routes) - [Set and get a cookie](#set-and-get-a-cookie) - - [Don't trust all proxies](#do-not-trust-all-proxies) + - [Don't trust all proxies](#dont-trust-all-proxies) - [Testing](#testing) - [Users](#users) @@ -2197,7 +2197,7 @@ func main() { } ``` -## Do not trust all proxies +## Don't trust all proxies 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 From 48d71d8b8dc5ff726f255646984b2f561cd9ee4b Mon Sep 17 00:00:00 2001 From: Notealot <714804968@qq.com> Date: Mon, 29 Nov 2021 12:25:21 +0800 Subject: [PATCH 08/10] format defaultTrustedCIDRs --- gin.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/gin.go b/gin.go index 685244411a..cd63b3e9be 100644 --- a/gin.go +++ b/gin.go @@ -29,8 +29,11 @@ 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 - { // ::/0 + { // 0.0.0.0/0 (IPv4) + IP: net.IP{0x0, 0x0, 0x0, 0x0}, + Mask: net.IPMask{0x0, 0x0, 0x0, 0x0}, + }, + { // ::/0 (IPv6) IP: net.IP{0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0}, Mask: net.IPMask{0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0}, }, From 839cc536f84cc79314288eaf0516df1a2aa8c62c Mon Sep 17 00:00:00 2001 From: Notealot <714804968@qq.com> Date: Mon, 29 Nov 2021 14:03:13 +0800 Subject: [PATCH 09/10] resolve conversation --- gin.go | 38 ++++++++++++++++++++------------------ 1 file changed, 20 insertions(+), 18 deletions(-) diff --git a/gin.go b/gin.go index cd63b3e9be..8170091b86 100644 --- a/gin.go +++ b/gin.go @@ -422,11 +422,12 @@ func (engine *Engine) parseTrustedProxies() error { // isTrustedProxy will check whether the IP address is included in the trusted list according to Engine.trustedCIDRs func (engine *Engine) isTrustedProxy(ip net.IP) bool { - if engine.trustedCIDRs != nil { - for _, cidr := range engine.trustedCIDRs { - if cidr.Contains(ip) { - return true - } + if engine.trustedCIDRs == nil { + return false + } + for _, cidr := range engine.trustedCIDRs { + if cidr.Contains(ip) { + return true } } return false @@ -434,20 +435,21 @@ func (engine *Engine) isTrustedProxy(ip net.IP) bool { // validateHeader will parse X-Forwarded-For header and return the trusted client IP address func (engine *Engine) validateHeader(header string) (clientIP string, valid bool) { - if header != "" { - items := strings.Split(header, ",") - for i := len(items) - 1; i >= 0; i-- { - ipStr := strings.TrimSpace(items[i]) - ip := net.ParseIP(ipStr) - if ip == nil { - break - } + if header == "" { + return "", false + } + items := strings.Split(header, ",") + for i := len(items) - 1; i >= 0; i-- { + ipStr := strings.TrimSpace(items[i]) + ip := net.ParseIP(ipStr) + if ip == nil { + break + } - // X-Forwarded-For is appended by proxy - // Check IPs in reverse order and stop when find untrusted proxy - if (i == 0) || (!engine.isTrustedProxy(ip)) { - return ipStr, true - } + // X-Forwarded-For is appended by proxy + // Check IPs in reverse order and stop when find untrusted proxy + if (i == 0) || (!engine.isTrustedProxy(ip)) { + return ipStr, true } } return "", false From ddc5c55d513c2ffc162c756e5f0099e61a0c3e36 Mon Sep 17 00:00:00 2001 From: Notealot <714804968@qq.com> Date: Mon, 29 Nov 2021 14:44:18 +0800 Subject: [PATCH 10/10] improve isUnsafeTrustedProxies() logic --- gin.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/gin.go b/gin.go index 8170091b86..b0a5887752 100644 --- a/gin.go +++ b/gin.go @@ -11,7 +11,6 @@ import ( "net/http" "os" "path" - "reflect" "strings" "sync" @@ -408,9 +407,9 @@ func (engine *Engine) SetTrustedProxies(trustedProxies []string) error { return engine.parseTrustedProxies() } -// isUnsafeTrustedProxies compares Engine.trustedCIDRs and defaultTrustedCIDRs, it's not safe if equal (returns true) +// isUnsafeTrustedProxies checks if Engine.trustedCIDRs contains all IPs, it's not safe if it has (returns true) func (engine *Engine) isUnsafeTrustedProxies() bool { - return reflect.DeepEqual(engine.trustedCIDRs, defaultTrustedCIDRs) + return engine.isTrustedProxy(net.ParseIP("0.0.0.0")) || engine.isTrustedProxy(net.ParseIP("::")) } // parseTrustedProxies parse Engine.trustedProxies to Engine.trustedCIDRs