Repository: gin-gonic/gin
Pull request #2632 (2021-02-07 23:45:20)

feat(engine): add trustedproxies and remoteIP
TODO: 
- [x] add new tests
- [x] fix tests

codecov[bot]: # [Codecov](https://codecov.io/gh/gin-gonic/gin/pull/2632?src=pr&el=h1) Report
> Merging [#2632](https://codecov.io/gh/gin-gonic/gin/pull/2632?src=pr&el=desc) (31a2afa) into [master](https://codecov.io/gh/gin-gonic/gin/commit/e899771392ecf35de8ce10a030ed8fed2207e9cb?el=desc) (e899771) will **increase** coverage by `0.02%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/gin-gonic/gin/pull/2632/graphs/tree.svg?width=650&height=150&src=pr&token=Qpqxhwery8)](https://codecov.io/gh/gin-gonic/gin/pull/2632?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master    #2632      +/-   ##
==========================================
+ Coverage   98.64%   98.66%   +0.02%     
==========================================
  Files          41       41              
  Lines        1989     2026      +37     
==========================================
+ Hits         1962     1999      +37     
  Misses         15       15              
  Partials       12       12              
```


| [Impacted Files](https://codecov.io/gh/gin-gonic/gin/pull/2632?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [context.go](https://codecov.io/gh/gin-gonic/gin/pull/2632/diff?src=pr&el=tree#diff-Y29udGV4dC5nbw==) | `97.72% <100.00%> (+0.21%)` | :arrow_up: |
| [gin.go](https://codecov.io/gh/gin-gonic/gin/pull/2632/diff?src=pr&el=tree#diff-Z2luLmdv) | `99.02% <100.00%> (+0.01%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/gin-gonic/gin/pull/2632?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/gin-gonic/gin/pull/2632?src=pr&el=footer). Last update [e899771...c9ea8ec](https://codecov.io/gh/gin-gonic/gin/pull/2632?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

RainbowMango: Hi Forks, would you like to cut a new release after this PR?
javierprovecho: @RainbowMango we’ll work towards it, as this PR brings some new properties to `gin.engine`, the release should be 1.7.0, so I may need some sync with @appleboy and @thinkerou on that milestone.
RainbowMango: Thanks for your quick response. I'm looking forward to the new release.
manucorporat: Alright team.

Added a new public API called:
```
ip, trusted := c.RemoteIP()
```
this function parsed the remote IP, and checks if it's a trusted proxy OR not.

Then clientIP function logic focuses in using this information and parse the appropriated headers.

Should help to keep logic separated and easier to test as well as a interesting new API, that can be used by developers to use their own logic and for example "trust" other headers.

Thoughts?

zymzxq: Any update:)
norbertstrzelecki: Hi Folks, is there any rough estimate for this one? Thanks!
cloudysulphur: Is there any way we can help with this pull request and get an alpha version for 1.7.0?

The open CVE with 1.6.3 is becoming a problem because it is tagged as a high vulnerability.

I would rather help out to fix this CVE than move to another web-server library.
appleboy: Maybe I or @thinkerou can handle this PR.
appleboy: I added to the v1.7 milestone.
thinkerou: > Maybe I or @thinkerou can handle this PR.

OK, I'm doing it this weekend.
thinkerou: Because `trustedCIDRs` does not initialize on `New() *Engine`, here : https://github.com/gin-gonic/gin/pull/2632/files#diff-447bee2da23875fead662441fef4b89e1918d4b5f01d1aa3f5d6cdc0cc51a4acL129
thinkerou: @appleboy we merge the pr and publish v1.7 now? thanks!
appleboy: @thinkerou Yes, waiting for the final Travis report.

Patch:
diff --git a/README.md b/README.md
index eb84031..d4772d7 100644
--- a/README.md
+++ b/README.md
@@ -2124,6 +2124,39 @@ 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 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 (
+	"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 3d6b56d..1ba0fa2 100644
--- a/context.go
+++ b/context.go
@@ -725,32 +725,82 @@ 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.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 c.engine.AppEngine {
+		if addr := c.requestHeader("X-Appengine-Remote-Addr"); addr != "" {
+			return addr
 		}
-		if clientIP != "" {
-			return clientIP
+	}
+
+	remoteIP, trusted := c.RemoteIP()
+	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))
+			if valid {
+				return ip
+			}
 		}
 	}
+	return remoteIP.String()
+}
 
-	if c.engine.AppEngine {
-		if addr := c.requestHeader("X-Appengine-Remote-Addr"); addr != "" {
-			return addr
+// 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 {
+		return nil, false
+	}
+	remoteIP := net.ParseIP(ip)
+	if remoteIP == nil {
+		return nil, false
+	}
+
+	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
+			}
 		}
 	}
 
-	if ip, _, err := net.SplitHostPort(strings.TrimSpace(c.Request.RemoteAddr)); err == nil {
-		return ip
+	return remoteIP, false
+}
+
+func validateHeader(header string) (clientIP string, valid bool) {
+	if header == "" {
+		return "", false
 	}
+	items := strings.Split(header, ",")
+	for i, ipStr := range items {
+		ipStr = strings.TrimSpace(ipStr)
+		ip := net.ParseIP(ipStr)
+		if ip == nil {
+			return "", false
+		}
 
-	return ""
+		// 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
+		}
+	}
+	return
 }
 
 // ContentType returns the Content-Type header of the request.
diff --git a/gin.go b/gin.go
index 1e12617..03a0e12 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"
@@ -81,9 +82,26 @@ type Engine struct {
 	// If no other Method is allowed, the request is delegated to the NotFound
 	// handler.
 	HandleMethodNotAllowed bool
-	ForwardedByClientIP    bool
 
-	// #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, 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
 	// 'X-AppEngine...' for better integration with that PaaS.
 	AppEngine bool
 
@@ -114,6 +132,7 @@ type Engine struct {
 	pool             sync.Pool
 	trees            methodTrees
 	maxParams        uint16
+	trustedCIDRs     []*net.IPNet
 }
 
 var _ IRouter = &Engine{}
@@ -139,6 +158,8 @@ 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,
@@ -305,12 +326,60 @@ 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.prepareTrustedCIDRs()
+	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) prepareTrustedCIDRs() ([]*net.IPNet, error) {
+	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}
+			}
+
+			switch len(ip) {
+			case net.IPv4len:
+				trustedProxy += "/32"
+			case net.IPv6len:
+				trustedProxy += "/128"
+			}
+		}
+		_, cidrNet, err := net.ParseCIDR(trustedProxy)
+		if err != nil {
+			return cidr, err
+		}
+		cidr = append(cidr, cidrNet)
+	}
+	return cidr, 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.


Test patch:
diff --git a/context_test.go b/context_test.go
index 8e1e3b5..8fe4761 100644
--- a/context_test.go
+++ b/context_test.go
@@ -1392,11 +1392,10 @@ 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 "
+	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")
@@ -1416,6 +1415,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, "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"}
+	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, "40.40.40.40", 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) {
@@ -1960,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)
+}
diff --git a/gin_integration_test.go b/gin_integration_test.go
index 41ad987..fd97265 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 11bdd79..678d95f 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,139 @@ 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)
+	}
+
+	// nil value
+	{
+		r.TrustedProxies = nil
+		trustedCIDRs, err := r.prepareTrustedCIDRs()
+
+		assert.Nil(t, trustedCIDRs)
+		assert.Nil(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 {