From de0f9eb3380f1090ef555df4abc4ee71d32985f5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B8ren=20L=2E=20Hansen?= Date: Thu, 20 Aug 2020 23:51:33 +0200 Subject: [PATCH 01/16] X-Forwarded-For handling is unsafe Breaks API, but immensely improves security Fixes #2473 --- README.md | 37 ++++++++++++++++++++ context.go | 91 +++++++++++++++++++++++++++++++++++++++++++------ context_test.go | 88 ++++++++++++++++++++++++++++++++++++++++++++--- gin.go | 7 ++++ 4 files changed, 208 insertions(+), 15 deletions(-) diff --git a/README.md b/README.md index fc1d01cf3f..74725ee0fc 100644 --- a/README.md +++ b/README.md @@ -2115,6 +2115,43 @@ func main() { } ``` +## 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 +specify one of these headers. + +The `TrustedProxies` slice on your `gin.Engine` specifes the clients +truest to specify unspoofed client IP headers. Proxies can be specified +as IP's, CIDR's, or hostnames. Hostnames are resolved on each query, +such that changes in your proxy pool take effect immediately. The +hostname option is handy, but also costly, so only use if you have no +other option. + +```go +import ( + "fmt" + + "github.com/gin-gonic/gin" +) + +func main() { + + router := gin.Default() + router.TrustedProxies = []string{"192.168.1.2"} + + router.GET("/", func(c *gin.Context) { + + // If the client is 192.168.1.2, use the X-Forwarded-For + // header to deduce the original client IP from the trust- + // worthy parts of that header. + // Otherwise, simply return the direct client IP + fmt.Printf("ClientIP: %s\n", c.ClientIP()) + }) + + router.Run() +} +``` ## Testing diff --git a/context.go b/context.go index 95b1807d72..2b33c207cc 100644 --- a/context.go +++ b/context.go @@ -713,14 +713,12 @@ func (c *Context) ShouldBindBodyWith(obj interface{}, bb binding.BindingBody) (e // X-Real-IP and X-Forwarded-For in order to work properly with reverse-proxies such us: nginx or haproxy. // Use X-Forwarded-For before X-Real-Ip as nginx uses X-Real-Ip with the proxy's IP. func (c *Context) ClientIP() string { - if c.engine.ForwardedByClientIP { - clientIP := c.requestHeader("X-Forwarded-For") - clientIP = strings.TrimSpace(strings.Split(clientIP, ",")[0]) - if clientIP == "" { - clientIP = strings.TrimSpace(c.requestHeader("X-Real-Ip")) - } - if clientIP != "" { - return clientIP + if c.engine.ForwardedByClientIP && c.engine.RemoteIPHeaders != nil { + for _, header := range c.engine.RemoteIPHeaders { + ipChain := filterIPsFromUntrustedProxies(c.requestHeader(header), c.Request, c.engine) + if len(ipChain) > 0 { + return ipChain[0] + } } } @@ -730,11 +728,82 @@ func (c *Context) ClientIP() string { } } - if ip, _, err := net.SplitHostPort(strings.TrimSpace(c.Request.RemoteAddr)); err == nil { - return ip + ip, _ := getTransportPeerIPForRequest(c.Request) + + return ip +} + +func filterIPsFromUntrustedProxies(XForwardedForHeader string, req *http.Request, e *Engine) []string { + var items, out []string + if XForwardedForHeader != "" { + items = strings.Split(XForwardedForHeader, ",") + } else { + return []string{} + } + if peerIP, err := getTransportPeerIPForRequest(req); err == nil { + items = append(items, peerIP) } - return "" + for i := len(items) - 1; i >= 0; i-- { + item := strings.TrimSpace(items[i]) + ip := net.ParseIP(item) + if ip == nil { + return out + } + + out = prependString(ip.String(), out) + if !isTrustedProxy(ip, e) { + return out + } + // out = prependString(ip.String(), out) + } + return out +} + +func isTrustedProxy(ip net.IP, e *Engine) bool { + for _, trustedProxy := range e.TrustedProxies { + if _, ipnet, err := net.ParseCIDR(trustedProxy); err == nil { + if ipnet.Contains(ip) { + return true + } + continue + } + + if proxyIP := net.ParseIP(trustedProxy); proxyIP != nil { + if proxyIP.Equal(ip) { + return true + } + continue + } + + if addrs, err := e.lookupHost(trustedProxy); err == nil { + for _, proxyAddr := range addrs { + proxyIP := net.ParseIP(proxyAddr) + if proxyIP == nil { + continue + } + if proxyIP.Equal(ip) { + return true + } + } + } + } + return false +} + +func prependString(ip string, ipList []string) []string { + ipList = append(ipList, "") + copy(ipList[1:], ipList) + ipList[0] = string(ip) + return ipList +} + +func getTransportPeerIPForRequest(req *http.Request) (string, error) { + var err error + if ip, _, err := net.SplitHostPort(strings.TrimSpace(req.RemoteAddr)); err == nil { + return ip, nil + } + return "", err } // ContentType returns the Content-Type header of the request. diff --git a/context_test.go b/context_test.go index 1a5a3c5ff5..7ffc3aafb0 100644 --- a/context_test.go +++ b/context_test.go @@ -1380,11 +1380,23 @@ func TestContextClientIP(t *testing.T) { c, _ := CreateTestContext(httptest.NewRecorder()) c.Request, _ = http.NewRequest("POST", "/", nil) - c.Request.Header.Set("X-Real-IP", " 10.10.10.10 ") - c.Request.Header.Set("X-Forwarded-For", " 20.20.20.20, 30.30.30.30") - c.Request.Header.Set("X-Appengine-Remote-Addr", "50.50.50.50") - c.Request.RemoteAddr = " 40.40.40.40:42123 " + c.engine.lookupHost = func(host string) ([]string, error) { + if host == "foo" { + return []string{"40.40.40.40", "30.30.30.30"}, nil + } + if host == "bar" { + return nil, errors.New("hostname lookup failed") + } + if host == "baz" { + return []string{"thisshouldneverhappen"}, nil + } + return []string{}, nil + } + + resetContextForClientIPTests(c) + // Legacy tests (validating that the defaults don't break the + // (insecure!) old behaviour) assert.Equal(t, "20.20.20.20", c.ClientIP()) c.Request.Header.Del("X-Forwarded-For") @@ -1404,6 +1416,74 @@ func TestContextClientIP(t *testing.T) { // no port c.Request.RemoteAddr = "50.50.50.50" assert.Empty(t, c.ClientIP()) + + // Tests exercising the TrustedProxies functionality + resetContextForClientIPTests(c) + + // No trusted proxies + c.engine.TrustedProxies = []string{} + c.engine.RemoteIPHeaders = []string{"X-Forwarded-For"} + assert.Equal(t, "40.40.40.40", c.ClientIP()) + + // Last proxy is trusted, but the RemoteAddr is not + c.engine.TrustedProxies = []string{"30.30.30.30"} + assert.Equal(t, "40.40.40.40", c.ClientIP()) + + // Only trust RemoteAddr + c.engine.TrustedProxies = []string{"40.40.40.40"} + assert.Equal(t, "30.30.30.30", c.ClientIP()) + + // All steps are trusted + c.engine.TrustedProxies = []string{"40.40.40.40", "30.30.30.30", "20.20.20.20"} + assert.Equal(t, "20.20.20.20", c.ClientIP()) + + // Use CIDR + c.engine.TrustedProxies = []string{"40.40.25.25/16", "30.30.30.30"} + assert.Equal(t, "20.20.20.20", c.ClientIP()) + + // Use hostname that resolves to all the proxies + c.engine.TrustedProxies = []string{"foo"} + assert.Equal(t, "20.20.20.20", c.ClientIP()) + + // Use hostname that returns an error + c.engine.TrustedProxies = []string{"bar"} + assert.Equal(t, "40.40.40.40", c.ClientIP()) + + // X-Forwarded-For has a non-IP element + c.engine.TrustedProxies = []string{"40.40.40.40"} + c.Request.Header.Set("X-Forwarded-For", " blah ") + assert.Equal(t, "40.40.40.40", c.ClientIP()) + + // Result from LookupHost has non-IP element. This should never + // happen, but we should test it to make sure we handle it + // gracefully. + c.engine.TrustedProxies = []string{"baz"} + c.Request.Header.Set("X-Forwarded-For", " 30.30.30.30 ") + assert.Equal(t, "40.40.40.40", c.ClientIP()) + + c.engine.TrustedProxies = []string{"40.40.40.40"} + c.Request.Header.Del("X-Forwarded-For") + c.engine.RemoteIPHeaders = []string{"X-Forwarded-For", "X-Real-IP"} + assert.Equal(t, "10.10.10.10", c.ClientIP()) + + c.engine.RemoteIPHeaders = []string{} + c.engine.AppEngine = true + assert.Equal(t, "50.50.50.50", c.ClientIP()) + + c.Request.Header.Del("X-Appengine-Remote-Addr") + assert.Equal(t, "40.40.40.40", c.ClientIP()) + + // no port + c.Request.RemoteAddr = "50.50.50.50" + assert.Empty(t, c.ClientIP()) +} + +func resetContextForClientIPTests(c *Context) { + c.Request.Header.Set("X-Real-IP", " 10.10.10.10 ") + c.Request.Header.Set("X-Forwarded-For", " 20.20.20.20, 30.30.30.30") + c.Request.Header.Set("X-Appengine-Remote-Addr", "50.50.50.50") + c.Request.RemoteAddr = " 40.40.40.40:42123 " + c.engine.AppEngine = false } func TestContextContentType(t *testing.T) { diff --git a/gin.go b/gin.go index 1e12617904..876bfe8a69 100644 --- a/gin.go +++ b/gin.go @@ -82,6 +82,8 @@ type Engine struct { // handler. HandleMethodNotAllowed bool ForwardedByClientIP bool + RemoteIPHeaders []string + TrustedProxies []string // #726 #755 If enabled, it will thrust some headers starting with // 'X-AppEngine...' for better integration with that PaaS. @@ -103,6 +105,8 @@ type Engine struct { // See the PR #1817 and issue #1644 RemoveExtraSlash bool + lookupHost func(string) ([]string, error) + delims render.Delims secureJSONPrefix string HTMLRender render.HTMLRender @@ -139,11 +143,14 @@ func New() *Engine { RedirectFixedPath: false, HandleMethodNotAllowed: false, ForwardedByClientIP: true, + RemoteIPHeaders: []string{"X-Forwarded-For", "X-Real-IP"}, + TrustedProxies: []string{"0.0.0.0/0"}, AppEngine: defaultAppEngine, UseRawPath: false, RemoveExtraSlash: false, UnescapePathValues: true, MaxMultipartMemory: defaultMultipartMemory, + lookupHost: net.LookupHost, trees: make(methodTrees, 0, 9), delims: render.Delims{Left: "{{", Right: "}}"}, secureJSONPrefix: "while(1);", From c9ea8ece4a3881028f7f715f008414346a7f4b88 Mon Sep 17 00:00:00 2001 From: "Manu Mtz.-Almeida" Date: Mon, 8 Feb 2021 00:43:37 +0100 Subject: [PATCH 02/16] refactor(): fix client Ip vulnerability --- context.go | 112 ++++++++++++++++++----------------------------------- gin.go | 25 ++++++++++++ 2 files changed, 63 insertions(+), 74 deletions(-) diff --git a/context.go b/context.go index 9e7a65782d..9fa3cb8190 100644 --- a/context.go +++ b/context.go @@ -729,13 +729,13 @@ func (c *Context) ShouldBindBodyWith(obj interface{}, bb binding.BindingBody) (e // X-Real-IP and X-Forwarded-For in order to work properly with reverse-proxies such us: nginx or haproxy. // Use X-Forwarded-For before X-Real-Ip as nginx uses X-Real-Ip with the proxy's IP. func (c *Context) ClientIP() string { - if c.engine.ForwardedByClientIP && c.engine.RemoteIPHeaders != nil { - for _, header := range c.engine.RemoteIPHeaders { - ipChain := filterIPsFromUntrustedProxies(c.requestHeader(header), c.Request, c.engine) - if len(ipChain) > 0 { - return ipChain[0] - } - } + ip, _, err := net.SplitHostPort(strings.TrimSpace(c.Request.RemoteAddr)) + if err != nil { + return "" + } + remoteIP := net.ParseIP(ip) + if remoteIP == nil { + return "" } if c.engine.AppEngine { @@ -744,82 +744,46 @@ func (c *Context) ClientIP() string { } } - ip, _ := getTransportPeerIPForRequest(c.Request) - - return ip -} - -func filterIPsFromUntrustedProxies(XForwardedForHeader string, req *http.Request, e *Engine) []string { - var items, out []string - if XForwardedForHeader != "" { - items = strings.Split(XForwardedForHeader, ",") - } else { - return []string{} - } - if peerIP, err := getTransportPeerIPForRequest(req); err == nil { - items = append(items, peerIP) - } - - for i := len(items) - 1; i >= 0; i-- { - item := strings.TrimSpace(items[i]) - ip := net.ParseIP(item) - if ip == nil { - return out - } - - out = prependString(ip.String(), out) - if !isTrustedProxy(ip, e) { - return out - } - // out = prependString(ip.String(), out) - } - return out -} - -func isTrustedProxy(ip net.IP, e *Engine) bool { - for _, trustedProxy := range e.TrustedProxies { - if _, ipnet, err := net.ParseCIDR(trustedProxy); err == nil { - if ipnet.Contains(ip) { - return true - } - continue - } - - if proxyIP := net.ParseIP(trustedProxy); proxyIP != nil { - if proxyIP.Equal(ip) { - return true - } - continue - } - - if addrs, err := e.lookupHost(trustedProxy); err == nil { - for _, proxyAddr := range addrs { - proxyIP := net.ParseIP(proxyAddr) - if proxyIP == nil { - continue - } - if proxyIP.Equal(ip) { - return true + if c.shouldCheckIPHeaders() { + for _, cidr := range c.engine.trustedCIDRs { + if cidr.Contains(remoteIP) { + for _, headerName := range c.engine.RemoteIPHeaders { + ip, valid := validateHeader(c.requestHeader(headerName)) + if valid { + return ip + } } } } } - return false + + return remoteIP.String() } -func prependString(ip string, ipList []string) []string { - ipList = append(ipList, "") - copy(ipList[1:], ipList) - ipList[0] = string(ip) - return ipList +func (c *Context) shouldCheckIPHeaders() bool { + return c.engine.ForwardedByClientIP && + c.engine.RemoteIPHeaders != nil && + len(c.engine.RemoteIPHeaders) > 0 && + c.engine.trustedCIDRs != nil } -func getTransportPeerIPForRequest(req *http.Request) (string, error) { - var err error - if ip, _, err := net.SplitHostPort(strings.TrimSpace(req.RemoteAddr)); err == nil { - return ip, nil +func validateHeader(header string) (clientIP string, valid bool) { + if header == "" { + return + } + items := strings.Split(header, ",") + for i, ipStr := range items { + ipStr = strings.TrimSpace(ipStr) + ip := net.ParseIP(ipStr) + if ip == nil { + return "", false + } + if i == 0 { + clientIP = ipStr + valid = true + } } - return "", err + return } // ContentType returns the Content-Type header of the request. diff --git a/gin.go b/gin.go index 876bfe8a69..877d3238b9 100644 --- a/gin.go +++ b/gin.go @@ -11,6 +11,7 @@ import ( "net/http" "os" "path" + "strings" "sync" "github.com/gin-gonic/gin/internal/bytesconv" @@ -118,6 +119,7 @@ type Engine struct { pool sync.Pool trees methodTrees maxParams uint16 + trustedCIDRs []*net.IPNet } var _ IRouter = &Engine{} @@ -312,12 +314,35 @@ func iterate(path, method string, routes RoutesInfo, root *node) RoutesInfo { func (engine *Engine) Run(addr ...string) (err error) { defer func() { debugPrintError(err) }() + trustedCIDRs, err := engine.prepareCIDR() + if err != nil { + return err + } + engine.trustedCIDRs = trustedCIDRs address := resolveAddress(addr) debugPrint("Listening and serving HTTP on %s\n", address) err = http.ListenAndServe(address, engine) return } +func (engine *Engine) prepareCIDR() ([]*net.IPNet, error) { + if engine.TrustedProxies != nil { + cidr := make([]*net.IPNet, len(engine.TrustedProxies), 0) + for _, trustedProxy := range engine.TrustedProxies { + if strings.Contains(trustedProxy, "/") { + trustedProxy += "/32" + } + _, cidrNet, err := net.ParseCIDR(trustedProxy) + if err != nil { + return cidr, err + } + cidr = append(cidr, cidrNet) + } + return cidr, nil + } + return nil, nil +} + // RunTLS attaches the router to a http.Server and starts listening and serving HTTPS (secure) requests. // It is a shortcut for http.ListenAndServeTLS(addr, certFile, keyFile, router) // Note: this method will block the calling goroutine indefinitely unless an error happens. From 39b372f1635157c1430e64d07798313ee8c576b3 Mon Sep 17 00:00:00 2001 From: Javier Provecho Fernandez Date: Mon, 8 Feb 2021 00:06:59 +0000 Subject: [PATCH 03/16] chore(engine): remove unused lookupHost --- context_test.go | 13 ------------- gin.go | 3 --- 2 files changed, 16 deletions(-) diff --git a/context_test.go b/context_test.go index 16c4dd2861..cb1f9c5bb5 100644 --- a/context_test.go +++ b/context_test.go @@ -1392,19 +1392,6 @@ func TestContextClientIP(t *testing.T) { c, _ := CreateTestContext(httptest.NewRecorder()) c.Request, _ = http.NewRequest("POST", "/", nil) - c.engine.lookupHost = func(host string) ([]string, error) { - if host == "foo" { - return []string{"40.40.40.40", "30.30.30.30"}, nil - } - if host == "bar" { - return nil, errors.New("hostname lookup failed") - } - if host == "baz" { - return []string{"thisshouldneverhappen"}, nil - } - return []string{}, nil - } - resetContextForClientIPTests(c) // Legacy tests (validating that the defaults don't break the diff --git a/gin.go b/gin.go index 877d3238b9..d20fcfbf56 100644 --- a/gin.go +++ b/gin.go @@ -106,8 +106,6 @@ type Engine struct { // See the PR #1817 and issue #1644 RemoveExtraSlash bool - lookupHost func(string) ([]string, error) - delims render.Delims secureJSONPrefix string HTMLRender render.HTMLRender @@ -152,7 +150,6 @@ func New() *Engine { RemoveExtraSlash: false, UnescapePathValues: true, MaxMultipartMemory: defaultMultipartMemory, - lookupHost: net.LookupHost, trees: make(methodTrees, 0, 9), delims: render.Delims{Left: "{{", Right: "}}"}, secureJSONPrefix: "while(1);", From e14a43cc4c168d62f9b8be6139a2664e70098197 Mon Sep 17 00:00:00 2001 From: Javier Provecho Fernandez Date: Mon, 8 Feb 2021 00:17:21 +0000 Subject: [PATCH 04/16] fix(engine): wrong syntax/behaviour at prepareCIDR --- gin.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/gin.go b/gin.go index d20fcfbf56..57a96de475 100644 --- a/gin.go +++ b/gin.go @@ -324,9 +324,9 @@ func (engine *Engine) Run(addr ...string) (err error) { func (engine *Engine) prepareCIDR() ([]*net.IPNet, error) { if engine.TrustedProxies != nil { - cidr := make([]*net.IPNet, len(engine.TrustedProxies), 0) + cidr := make([]*net.IPNet, 0, len(engine.TrustedProxies)) for _, trustedProxy := range engine.TrustedProxies { - if strings.Contains(trustedProxy, "/") { + if !strings.Contains(trustedProxy, "/") { trustedProxy += "/32" } _, cidrNet, err := net.ParseCIDR(trustedProxy) From 55ad88a12b502cd91140b1d5c2fe745689c46b2f Mon Sep 17 00:00:00 2001 From: "Manu Mtz.-Almeida" Date: Mon, 8 Feb 2021 14:08:35 +0100 Subject: [PATCH 05/16] refactor move logic to remoteIP() --- context.go | 50 +++++++++++++++++++++++++++++--------------------- 1 file changed, 29 insertions(+), 21 deletions(-) diff --git a/context.go b/context.go index 9fa3cb8190..470a755641 100644 --- a/context.go +++ b/context.go @@ -729,42 +729,50 @@ func (c *Context) ShouldBindBodyWith(obj interface{}, bb binding.BindingBody) (e // X-Real-IP and X-Forwarded-For in order to work properly with reverse-proxies such us: nginx or haproxy. // Use X-Forwarded-For before X-Real-Ip as nginx uses X-Real-Ip with the proxy's IP. func (c *Context) ClientIP() string { - ip, _, err := net.SplitHostPort(strings.TrimSpace(c.Request.RemoteAddr)) - if err != nil { - return "" - } - remoteIP := net.ParseIP(ip) - if remoteIP == nil { - return "" - } - if c.engine.AppEngine { if addr := c.requestHeader("X-Appengine-Remote-Addr"); addr != "" { return addr } } - if c.shouldCheckIPHeaders() { - for _, cidr := range c.engine.trustedCIDRs { - if cidr.Contains(remoteIP) { - for _, headerName := range c.engine.RemoteIPHeaders { - ip, valid := validateHeader(c.requestHeader(headerName)) - if valid { - return ip - } - } + remoteIP, trusted := c.RemoteIP() + if remoteIP == nil { + return "" + } + if trusted { + for _, headerName := range c.engine.RemoteIPHeaders { + ip, valid := validateHeader(c.requestHeader(headerName)) + if valid { + return ip } } } - return remoteIP.String() } -func (c *Context) shouldCheckIPHeaders() bool { - return c.engine.ForwardedByClientIP && +func (c *Context) RemoteIP() (net.IP, bool) { + 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 + } + + shouldCheckTrustedIP := c.engine.ForwardedByClientIP && c.engine.RemoteIPHeaders != nil && len(c.engine.RemoteIPHeaders) > 0 && c.engine.trustedCIDRs != nil + + if shouldCheckTrustedIP { + for _, cidr := range c.engine.trustedCIDRs { + if cidr.Contains(remoteIP) { + return remoteIP, true + } + } + } + return remoteIP, false } func validateHeader(header string) (clientIP string, valid bool) { From 6f562eafa86530e78ac7599a70bcaeef4180bba7 Mon Sep 17 00:00:00 2001 From: "Manu Mtz.-Almeida" Date: Mon, 8 Feb 2021 14:19:09 +0100 Subject: [PATCH 06/16] add docs --- context.go | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/context.go b/context.go index 470a755641..9efaaabdb6 100644 --- a/context.go +++ b/context.go @@ -725,9 +725,11 @@ func (c *Context) ShouldBindBodyWith(obj interface{}, bb binding.BindingBody) (e return bb.BindBody(body, obj) } -// ClientIP implements a best effort algorithm to return the real client IP, it parses -// X-Real-IP and X-Forwarded-For in order to work properly with reverse-proxies such us: nginx or haproxy. -// Use X-Forwarded-For before X-Real-Ip as nginx uses X-Real-Ip with the proxy's IP. +// ClientIP implements a best effort algorithm to return the real client IP. +// It called c.RemoteIP() under the hood, to check if the remote IP is a trusted proxy or not. +// If it's it will then try to parse the headers defined in Engine.RemoteIPHeaders (defaulting to [X-Forwarded-For, X-Real-Ip]). +// If the headers are nots syntactically valid OR the remote IP does not correspong to a trusted proxy, +// the remote IP (coming form Request.RemoteAddr) is returned. func (c *Context) ClientIP() string { if c.engine.AppEngine { if addr := c.requestHeader("X-Appengine-Remote-Addr"); addr != "" { @@ -739,7 +741,7 @@ func (c *Context) ClientIP() string { if remoteIP == nil { return "" } - if trusted { + if trusted && c.engine.ForwardedByClientIP && c.engine.RemoteIPHeaders != nil { for _, headerName := range c.engine.RemoteIPHeaders { ip, valid := validateHeader(c.requestHeader(headerName)) if valid { @@ -750,6 +752,10 @@ func (c *Context) ClientIP() string { return remoteIP.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 func (c *Context) RemoteIP() (net.IP, bool) { ip, _, err := net.SplitHostPort(strings.TrimSpace(c.Request.RemoteAddr)) if err != nil { @@ -759,13 +765,7 @@ func (c *Context) RemoteIP() (net.IP, bool) { if remoteIP == nil { return nil, false } - - shouldCheckTrustedIP := c.engine.ForwardedByClientIP && - c.engine.RemoteIPHeaders != nil && - len(c.engine.RemoteIPHeaders) > 0 && - c.engine.trustedCIDRs != nil - - if shouldCheckTrustedIP { + if c.engine.trustedCIDRs != nil { for _, cidr := range c.engine.trustedCIDRs { if cidr.Contains(remoteIP) { return remoteIP, true From ffe7ac09d07dbe894d0fa20a36ce793ee04d93a0 Mon Sep 17 00:00:00 2001 From: "Manu Mtz.-Almeida" Date: Mon, 8 Feb 2021 14:22:58 +0100 Subject: [PATCH 07/16] =?UTF-8?q?=F0=9F=90=88?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- README.md | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index 0289d1a2eb..e06ab992b0 100644 --- a/README.md +++ b/README.md @@ -2143,15 +2143,13 @@ func main() { router.TrustedProxies = []string{"192.168.1.2"} router.GET("/", func(c *gin.Context) { - // If the client is 192.168.1.2, use the X-Forwarded-For // header to deduce the original client IP from the trust- // worthy parts of that header. // Otherwise, simply return the direct client IP - fmt.Printf("ClientIP: %s\n", c.ClientIP()) - }) - - router.Run() + fmt.Printf("ClientIP: %s\n", c.ClientIP()) + }) + router.Run() } ``` From 2d426b64c32b5054f21efc5741d32e526cf7ed7c Mon Sep 17 00:00:00 2001 From: "Manu Mtz.-Almeida" Date: Mon, 8 Feb 2021 14:24:48 +0100 Subject: [PATCH 08/16] =?UTF-8?q?add=20explanation=20=F0=9F=A6=80?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- context.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/context.go b/context.go index 9efaaabdb6..3e040761ad 100644 --- a/context.go +++ b/context.go @@ -777,7 +777,7 @@ func (c *Context) RemoteIP() (net.IP, bool) { func validateHeader(header string) (clientIP string, valid bool) { if header == "" { - return + return "", false } items := strings.Split(header, ",") for i, ipStr := range items { @@ -786,6 +786,10 @@ func validateHeader(header string) (clientIP string, valid bool) { if ip == nil { return "", false } + + // We need to return the first IP in the list, but, + // we should not early return since we need to validate that + // the rest of the header is syntactically valid if i == 0 { clientIP = ipStr valid = true From fc99953b7fb9e4d167187e2a9b9c81f915257983 Mon Sep 17 00:00:00 2001 From: "Manu Mtz.-Almeida" Date: Mon, 8 Feb 2021 14:26:12 +0100 Subject: [PATCH 09/16] =?UTF-8?q?fix=20=F0=9F=90=A8?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- README.md | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index e06ab992b0..2b45edbcf8 100644 --- a/README.md +++ b/README.md @@ -2132,9 +2132,9 @@ other option. ```go import ( - "fmt" + "fmt" - "github.com/gin-gonic/gin" + "github.com/gin-gonic/gin" ) func main() { @@ -2142,14 +2142,14 @@ func main() { router := gin.Default() router.TrustedProxies = []string{"192.168.1.2"} - router.GET("/", func(c *gin.Context) { + router.GET("/", func(c *gin.Context) { // If the client is 192.168.1.2, use the X-Forwarded-For // header to deduce the original client IP from the trust- // worthy parts of that header. // Otherwise, simply return the direct client IP - fmt.Printf("ClientIP: %s\n", c.ClientIP()) - }) - router.Run() + fmt.Printf("ClientIP: %s\n", c.ClientIP()) + }) + router.Run() } ``` From 15f576c362b24578eeb4bc8467af4877af15450d Mon Sep 17 00:00:00 2001 From: Javier Provecho Fernandez Date: Tue, 9 Feb 2021 00:11:27 +0100 Subject: [PATCH 10/16] docs(readme): remove old reference to hostnames ... as trusted proxies --- README.md | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index 2b45edbcf8..dc7cf4246c 100644 --- a/README.md +++ b/README.md @@ -2124,11 +2124,8 @@ 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 the clients -truest to specify unspoofed client IP headers. Proxies can be specified -as IP's, CIDR's, or hostnames. Hostnames are resolved on each query, -such that changes in your proxy pool take effect immediately. The -hostname option is handy, but also costly, so only use if you have no -other option. +trusted to specify unspoofed client IP headers. Proxies can be specified +as IPs or CIDRs. ```go import ( From a598663fde61735f30bb5920e6a527a6228272fb Mon Sep 17 00:00:00 2001 From: Javier Provecho Fernandez Date: Tue, 9 Feb 2021 20:31:20 +0000 Subject: [PATCH 11/16] docs(engine): explain new gin.Engine properties --- gin.go | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/gin.go b/gin.go index 57a96de475..f178b1f097 100644 --- a/gin.go +++ b/gin.go @@ -82,11 +82,25 @@ type Engine struct { // If no other Method is allowed, the request is delegated to the NotFound // handler. HandleMethodNotAllowed bool - ForwardedByClientIP bool - RemoteIPHeaders []string - TrustedProxies []string - // #726 #755 If enabled, it will thrust some headers starting with + // If enabled, client IP will be parsed from the request's headers that + // match those stored at `(*gin.Engine).RemoteIPHeaders`. If no IP was + // fetched, it falls back to the IP obtained from + // `(*gin.Context).Request.RemoteAddr`. + ForwardedByClientIP bool + + // 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`. + RemoteIPHeaders []string + + // List of network origins (IPv4 addresses, IPv4 CIDRs or IPv6 addresses) + // from which to trust request's headers that contain alternative client + // IP when `(*gin.Engine).ForwardedByClientIP` is `true`. + TrustedProxies []string + + // #726 #755 If enabled, it will trust some headers starting with // 'X-AppEngine...' for better integration with that PaaS. AppEngine bool From 7e649c347fed4f4322d5a3eb893b8eff17c6d843 Mon Sep 17 00:00:00 2001 From: Javier Provecho Fernandez Date: Wed, 10 Feb 2021 01:20:44 +0000 Subject: [PATCH 12/16] feat(engine): add ipv6 support to TrustedProxies --- README.md | 7 +-- gin.go | 37 +++++++++++++--- gin_test.go | 125 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 160 insertions(+), 9 deletions(-) diff --git a/README.md b/README.md index dc7cf4246c..5ea266ade1 100644 --- a/README.md +++ b/README.md @@ -2123,9 +2123,10 @@ 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 the clients -trusted to specify unspoofed client IP headers. Proxies can be specified -as IPs or CIDRs. +The `TrustedProxies` slice on your `gin.Engine` specifes 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. ```go import ( diff --git a/gin.go b/gin.go index f178b1f097..2058c2d52b 100644 --- a/gin.go +++ b/gin.go @@ -95,9 +95,10 @@ type Engine struct { // network origins of `(*gin.Engine).TrustedProxies`. RemoteIPHeaders []string - // List of network origins (IPv4 addresses, IPv4 CIDRs or IPv6 addresses) - // from which to trust request's headers that contain alternative client - // IP when `(*gin.Engine).ForwardedByClientIP` is `true`. + // 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 // #726 #755 If enabled, it will trust some headers starting with @@ -325,7 +326,7 @@ func iterate(path, method string, routes RoutesInfo, root *node) RoutesInfo { func (engine *Engine) Run(addr ...string) (err error) { defer func() { debugPrintError(err) }() - trustedCIDRs, err := engine.prepareCIDR() + trustedCIDRs, err := engine.prepareTrustedCIDRs() if err != nil { return err } @@ -336,12 +337,22 @@ func (engine *Engine) Run(addr ...string) (err error) { return } -func (engine *Engine) prepareCIDR() ([]*net.IPNet, error) { +func (engine *Engine) prepareTrustedCIDRs() ([]*net.IPNet, error) { if engine.TrustedProxies != nil { cidr := make([]*net.IPNet, 0, len(engine.TrustedProxies)) for _, trustedProxy := range engine.TrustedProxies { if !strings.Contains(trustedProxy, "/") { - trustedProxy += "/32" + ip := parseIP(trustedProxy) + if ip == nil { + return cidr, &net.ParseError{Type: "IP address", Text: trustedProxy} + } + + switch len(ip) { + case net.IPv4len: + trustedProxy += "/32" + case net.IPv6len: + trustedProxy += "/128" + } } _, cidrNet, err := net.ParseCIDR(trustedProxy) if err != nil { @@ -354,6 +365,20 @@ func (engine *Engine) prepareCIDR() ([]*net.IPNet, error) { return nil, nil } +// 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 { + parsedIP := net.ParseIP(ip) + + if ipv4 := parsedIP.To4(); ipv4 != nil { + // return ip in a 4-byte representation + return ipv4 + } + + // return ip in a 16-byte representation or nil + return parsedIP +} + // RunTLS attaches the router to a http.Server and starts listening and serving HTTPS (secure) requests. // It is a shortcut for http.ListenAndServeTLS(addr, certFile, keyFile, router) // Note: this method will block the calling goroutine indefinitely unless an error happens. diff --git a/gin_test.go b/gin_test.go index 11bdd79ccf..75b13fd472 100644 --- a/gin_test.go +++ b/gin_test.go @@ -9,6 +9,7 @@ import ( "fmt" "html/template" "io/ioutil" + "net" "net/http" "net/http/httptest" "reflect" @@ -532,6 +533,130 @@ func TestEngineHandleContextManyReEntries(t *testing.T) { assert.Equal(t, int64(expectValue), middlewareCounter) } +func TestPrepareTrustedCIRDsWith(t *testing.T) { + r := New() + + // valid ipv4 cidr + { + expectedTrustedCIDRs := []*net.IPNet{parseCIDR("0.0.0.0/0")} + r.TrustedProxies = []string{"0.0.0.0/0"} + + trustedCIDRs, err := r.prepareTrustedCIDRs() + + assert.NoError(t, err) + assert.Equal(t, expectedTrustedCIDRs, trustedCIDRs) + } + + // invalid ipv4 cidr + { + r.TrustedProxies = []string{"192.168.1.33/33"} + + _, err := r.prepareTrustedCIDRs() + + 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() + + assert.NoError(t, err) + assert.Equal(t, expectedTrustedCIDRs, trustedCIDRs) + } + + // invalid ipv4 address + { + r.TrustedProxies = []string{"192.168.1.256"} + + _, err := r.prepareTrustedCIDRs() + + 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() + + assert.NoError(t, err) + assert.Equal(t, expectedTrustedCIDRs, trustedCIDRs) + } + + // invalid ipv6 address + { + r.TrustedProxies = []string{"gggg:0000:0000:1234:abcd:ffff:c0a8:0101"} + + _, err := r.prepareTrustedCIDRs() + + assert.Error(t, err) + } + + // valid ipv6 cidr + { + expectedTrustedCIDRs := []*net.IPNet{parseCIDR("::/0")} + r.TrustedProxies = []string{"::/0"} + + trustedCIDRs, err := r.prepareTrustedCIDRs() + + assert.NoError(t, err) + assert.Equal(t, expectedTrustedCIDRs, trustedCIDRs) + } + + // invalid ipv6 cidr + { + r.TrustedProxies = []string{"gggg:0000:0000:1234:abcd:ffff:c0a8:0101/129"} + + _, err := r.prepareTrustedCIDRs() + + assert.Error(t, err) + } + + // valid combination + { + expectedTrustedCIDRs := []*net.IPNet{ + parseCIDR("::/0"), + parseCIDR("192.168.0.0/16"), + parseCIDR("172.16.0.1/32"), + } + r.TrustedProxies = []string{ + "::/0", + "192.168.0.0/16", + "172.16.0.1", + } + + trustedCIDRs, err := r.prepareTrustedCIDRs() + + assert.NoError(t, err) + assert.Equal(t, expectedTrustedCIDRs, trustedCIDRs) + } + + // invalid combination + { + r.TrustedProxies = []string{ + "::/0", + "192.168.0.0/16", + "172.16.0.256", + } + _, err := r.prepareTrustedCIDRs() + + assert.Error(t, err) + } + +} + +func parseCIDR(cidr string) *net.IPNet { + _, parsedCIDR, err := net.ParseCIDR(cidr) + if err != nil { + fmt.Println(err) + } + return parsedCIDR +} + func assertRoutePresent(t *testing.T, gotRoutes RoutesInfo, wantRoute RouteInfo) { for _, gotRoute := range gotRoutes { if gotRoute.Path == wantRoute.Path && gotRoute.Method == wantRoute.Method { From 08766787f93ce6185e754a2c6955ede8a1447128 Mon Sep 17 00:00:00 2001 From: thinkerou Date: Sat, 27 Mar 2021 11:32:49 +0800 Subject: [PATCH 13/16] fix error --- context.go | 15 +++++++++++---- context_test.go | 4 ++-- gin.go | 43 ++++++++++++++++++++++--------------------- 3 files changed, 35 insertions(+), 27 deletions(-) diff --git a/context.go b/context.go index 3e040761ad..037ea8fe8c 100644 --- a/context.go +++ b/context.go @@ -741,6 +741,7 @@ func (c *Context) ClientIP() string { if remoteIP == nil { return "" } + if trusted && c.engine.ForwardedByClientIP && c.engine.RemoteIPHeaders != nil { for _, headerName := range c.engine.RemoteIPHeaders { ip, valid := validateHeader(c.requestHeader(headerName)) @@ -765,13 +766,19 @@ func (c *Context) RemoteIP() (net.IP, bool) { if remoteIP == nil { return nil, false } - if c.engine.trustedCIDRs != nil { - for _, cidr := range c.engine.trustedCIDRs { - if cidr.Contains(remoteIP) { - return remoteIP, true + + trustedCIDRs, err := c.engine.prepareTrustedCIDRs() + if err == nil { + c.engine.trustedCIDRs = trustedCIDRs + if c.engine.trustedCIDRs != nil { + for _, cidr := range c.engine.trustedCIDRs { + if cidr.Contains(remoteIP) { + return remoteIP, true + } } } } + return remoteIP, false } diff --git a/context_test.go b/context_test.go index cb1f9c5bb5..9570a9e9e7 100644 --- a/context_test.go +++ b/context_test.go @@ -1430,7 +1430,7 @@ func TestContextClientIP(t *testing.T) { // Only trust RemoteAddr c.engine.TrustedProxies = []string{"40.40.40.40"} - assert.Equal(t, "30.30.30.30", c.ClientIP()) + assert.Equal(t, "20.20.20.20", c.ClientIP()) // All steps are trusted c.engine.TrustedProxies = []string{"40.40.40.40", "30.30.30.30", "20.20.20.20"} @@ -1442,7 +1442,7 @@ func TestContextClientIP(t *testing.T) { // Use hostname that resolves to all the proxies c.engine.TrustedProxies = []string{"foo"} - assert.Equal(t, "20.20.20.20", c.ClientIP()) + assert.Equal(t, "40.40.40.40", c.ClientIP()) // Use hostname that returns an error c.engine.TrustedProxies = []string{"bar"} diff --git a/gin.go b/gin.go index 2058c2d52b..03a0e127e6 100644 --- a/gin.go +++ b/gin.go @@ -338,31 +338,32 @@ func (engine *Engine) Run(addr ...string) (err error) { } func (engine *Engine) prepareTrustedCIDRs() ([]*net.IPNet, error) { - if engine.TrustedProxies != nil { - cidr := make([]*net.IPNet, 0, len(engine.TrustedProxies)) - for _, trustedProxy := range engine.TrustedProxies { - if !strings.Contains(trustedProxy, "/") { - ip := parseIP(trustedProxy) - if ip == nil { - return cidr, &net.ParseError{Type: "IP address", Text: trustedProxy} - } - - switch len(ip) { - case net.IPv4len: - trustedProxy += "/32" - case net.IPv6len: - trustedProxy += "/128" - } + if engine.TrustedProxies == nil { + return nil, nil + } + + cidr := make([]*net.IPNet, 0, len(engine.TrustedProxies)) + for _, trustedProxy := range engine.TrustedProxies { + if !strings.Contains(trustedProxy, "/") { + ip := parseIP(trustedProxy) + if ip == nil { + return cidr, &net.ParseError{Type: "IP address", Text: trustedProxy} } - _, cidrNet, err := net.ParseCIDR(trustedProxy) - if err != nil { - return cidr, err + + switch len(ip) { + case net.IPv4len: + trustedProxy += "/32" + case net.IPv6len: + trustedProxy += "/128" } - cidr = append(cidr, cidrNet) } - return cidr, nil + _, cidrNet, err := net.ParseCIDR(trustedProxy) + if err != nil { + return cidr, err + } + cidr = append(cidr, cidrNet) } - return nil, nil + return cidr, nil } // parseIP parse a string representation of an IP and returns a net.IP with the From feaee20a8170683679f396d023697f4ab50d95af Mon Sep 17 00:00:00 2001 From: thinkerou Date: Sat, 27 Mar 2021 12:46:46 +0800 Subject: [PATCH 14/16] add unit test --- gin_integration_test.go | 7 +++++++ gin_test.go | 9 +++++++++ 2 files changed, 16 insertions(+) diff --git a/gin_integration_test.go b/gin_integration_test.go index 41ad98743b..fd972657cc 100644 --- a/gin_integration_test.go +++ b/gin_integration_test.go @@ -55,6 +55,13 @@ func TestRunEmpty(t *testing.T) { testRequest(t, "http://localhost:8080/example") } +func TestTrustedCIDRsForRun(t *testing.T) { + os.Setenv("PORT", "") + router := New() + router.TrustedProxies = []string{"hello/world"} + assert.Error(t, router.Run(":8080")) +} + func TestRunTLS(t *testing.T) { router := New() go func() { diff --git a/gin_test.go b/gin_test.go index 75b13fd472..678d95f2c8 100644 --- a/gin_test.go +++ b/gin_test.go @@ -647,6 +647,15 @@ func TestPrepareTrustedCIRDsWith(t *testing.T) { assert.Error(t, err) } + // nil value + { + r.TrustedProxies = nil + trustedCIDRs, err := r.prepareTrustedCIDRs() + + assert.Nil(t, trustedCIDRs) + assert.Nil(t, err) + } + } func parseCIDR(cidr string) *net.IPNet { From 2d7fc0696918031e1d88683c1e343d9658d88e0d Mon Sep 17 00:00:00 2001 From: thinkerou Date: Sat, 27 Mar 2021 13:59:31 +0800 Subject: [PATCH 15/16] add unit test --- context_test.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/context_test.go b/context_test.go index 9570a9e9e7..8fe4761570 100644 --- a/context_test.go +++ b/context_test.go @@ -2027,3 +2027,12 @@ func TestContextWithKeysMutex(t *testing.T) { assert.Nil(t, value) assert.False(t, err) } + +func TestRemoteIPFail(t *testing.T) { + c, _ := CreateTestContext(httptest.NewRecorder()) + c.Request, _ = http.NewRequest("POST", "/", nil) + c.Request.RemoteAddr = "[:::]:80" + ip, trust := c.RemoteIP() + assert.Nil(t, ip) + assert.False(t, trust) +} From 3483d2c07d8f9f41599a4c9f40448327191488af Mon Sep 17 00:00:00 2001 From: thinkerou Date: Sat, 27 Mar 2021 20:37:01 +0800 Subject: [PATCH 16/16] remove nil statement --- context.go | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/context.go b/context.go index 037ea8fe8c..1ba0fa2b0d 100644 --- a/context.go +++ b/context.go @@ -767,14 +767,12 @@ func (c *Context) RemoteIP() (net.IP, bool) { return nil, false } - trustedCIDRs, err := c.engine.prepareTrustedCIDRs() - if err == nil { - c.engine.trustedCIDRs = trustedCIDRs - if c.engine.trustedCIDRs != nil { - for _, cidr := range c.engine.trustedCIDRs { - if cidr.Contains(remoteIP) { - return remoteIP, true - } + trustedCIDRs, _ := c.engine.prepareTrustedCIDRs() + c.engine.trustedCIDRs = trustedCIDRs + if c.engine.trustedCIDRs != nil { + for _, cidr := range c.engine.trustedCIDRs { + if cidr.Contains(remoteIP) { + return remoteIP, true } } }