Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

c.ClientIP() #2697

Closed
artlevitan opened this issue Apr 20, 2021 · 49 comments
Closed

c.ClientIP() #2697

artlevitan opened this issue Apr 20, 2021 · 49 comments

Comments

@artlevitan
Copy link

artlevitan commented Apr 20, 2021

Friends, the update to 1.7 broke the work with IP addresses of clients.

I am proxying Go through Nginx

location /backend/ {
        rewrite /backend/(.*) /$1 break;
        proxy_set_header X-Forwarded-For $remote_addr;
        proxy_set_header X-Real-IP $remote_addr;
        proxy_set_header Host $http_host;
        proxy_redirect off;
        proxy_pass http://127.0.0.1:8080;
    }

I read this #2693

But I don't understand what to write in router.TrustedProxies

func GinRouter() *gin.Engine {
	router := gin.New()

	// Set a lower memory limit for multipart forms
	router.MaxMultipartMemory = 100 << 20 // 100 MB

	// Custom Logger
	router.Use(gin.LoggerWithFormatter(func(param gin.LogFormatterParams) string {
		return fmt.Sprintf("%s |%s %d %s| %s |%s %s %s %s | %s | Body: %s | %s | %s\n",
			param.TimeStamp.Format(time.RFC1123),
			param.StatusCodeColor(),
			param.StatusCode,
			param.ResetColor(),
			param.ClientIP,
			param.MethodColor(),
			param.Method,
			param.ResetColor(),
			param.Path,
			param.Latency,
			byteCountSI(param.BodySize),
			param.Request.UserAgent(),
			param.ErrorMessage,
		)
	}))

	// Recovery middleware recovers from any panics and writes a 500 if there was one.
	router.Use(gin.Recovery())

	return router
}

Thanks everyone 🙏🏻

@cyanBone
Copy link

me too

@1989bajie
Copy link

框架升级是不是需要考虑平滑?这就意味着用过这个方法的用户,升完级都要出问题了。。。。

@leungyauming
Copy link

框架升级是不是需要考虑平滑?这就意味着用过这个方法的用户,升完级都要出问题了。。。。

Translate:
Shouldn't migration be considered in upgrades? That means users who have used the method will be in trouble after upgrading.

@artlevitan
Copy link
Author

artlevitan commented Apr 27, 2021

The best way is roll back the trusted proxy update and make it optional.

Almost everyone uses Nginx and add-on packages so that there is no doubt about the client.

@leungyauming
Copy link

leungyauming commented Apr 29, 2021

// 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.

According to the documentation of the gin.Context.ClientIP() function, it will check if the remote IP (which is the ip of the proxy or the ip of nginx in your case) is a trusted IP or not.

If it is a trusted IP (which means the request is redirected by a proxy), then it will try to parse the "real user IP" from X-Forwarded-For/X-Real-Ip header.

Conclusion: You can think TrustedProxies as the identifiers for classifying whether the request is redirected from a proxy or not. If you don't type any IP into TrustedProxies, then you may get the IP of the proxy as the "user" IP.

For Chinese:
根據gin.Context.ClientIP()函數的文檔,該函數會先透過TrustedProxies變量來偵查HTTP請求的來源IP是否是所謂的“信任代理”。
如果該HTTP請求的來源IP屬於“信任代理”那麼伺服器就會嘗試從X-Forwarded-ForX-Real-Ip頭文取得真正用戶的IP。所以簡單來說就是,TrustedProxies是用來給伺服器判斷是否從頭文取得IP的。

@artlevitan
Copy link
Author

Thanks for the answer, I saw the documentation and code comments.

Why is 127.0.0.1 not the default trusted address? Most people use Nginx for serve to WAN.

@leungyauming
Copy link

leungyauming commented Apr 29, 2021

// New returns a new blank Engine instance without any middleware attached.
// By default the configuration is:
// - RedirectTrailingSlash:  true
// - RedirectFixedPath:      false
// - HandleMethodNotAllowed: false
// - ForwardedByClientIP:    true
// - UseRawPath:             false
// - UnescapePathValues:     true
func New() *Engine {
	debugPrintWARNINGNew()
	engine := &Engine{
		RouterGroup: RouterGroup{
			Handlers: nil,
			basePath: "/",
			root:     true,
		},
		FuncMap:                template.FuncMap{},
		RedirectTrailingSlash:  true,
		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,
		trees:                  make(methodTrees, 0, 9),
		delims:                 render.Delims{Left: "{{", Right: "}}"},
		secureJSONPrefix:       "while(1);",
	}
	engine.RouterGroup.engine = engine
	engine.pool.New = func() interface{} {
		return engine.allocateContext()
	}
	return engine
}

As you can see, the default value of the TrustedProxies field is []string{"0.0.0.0/0"} which should has included 127.0.0.1 already.

And alternative IP in X-Forwarded-For/X-Real-IP should be returned if they actually present.

@artlevitan
Copy link
Author

it does not work for me, as well as for many who opened topics with such a problem.

#2693

@leungyauming
Copy link

Interesting, let me go to take a look and test it.

@artlevitan
Copy link
Author

artlevitan commented Apr 29, 2021

log from SSH

main[25495]: Thu, 29 Apr 2021 12:16:51 | 200 | 127.0.0.1 | ....
main[25495]: Thu, 29 Apr 2021 12:16:53 | 200 | 127.0.0.1 | ....

In all responses localhost (127.0.0.1)

the proxy configuration was thrown above

location /backend/ {
        rewrite /backend/(.*) /$1 break;
        proxy_set_header X-Forwarded-For $remote_addr;
        proxy_set_header X-Real-IP $remote_addr;
        proxy_set_header Host $http_host;
        proxy_redirect off;
        proxy_pass http://127.0.0.1:8080;
    }

@leungyauming
Copy link

leungyauming commented Apr 29, 2021

client/main.go

package main

import (
	"fmt"
	"io"
	"net/http"
)

func main() {
	req, _ := http.NewRequest(http.MethodGet, "http://127.0.0.1:8080", nil)
	req.Header.Add("X-Forwarded-For", "123.123.123.123")
	res, _ := http.DefaultClient.Do(req)
	body := res.Body
	data, _ := io.ReadAll(body)
	fmt.Println(string(data))
	defer body.Close()
}

server/main.go

package main

import (
	"net/http"

	"github.com/gin-gonic/gin"
)

func main() {
	r := gin.New()

	r.GET("/", func(c *gin.Context) {
		c.String(http.StatusOK, "%s", c.ClientIP())
	})

	r.Run()
}

I created a simple client and server.
Then sent a http request with X-Forwarded-For header set to 123.123.123.123 from the client to the server.
Print the result to the console.

Screenshot 2021-04-29 at 5 33 26 PM

Here is the output. As you can see, the result is correct and the feature is working.
May you please check if the header is set and sent to the server successfully? (I suggest printing the headers received in your handler function, the variable name is c.Request.Header which should be a string-string map)

@artlevitan
Copy link
Author

artlevitan commented Apr 29, 2021

Header X-Forwarded-For from Nginx in my case is correct too
But c.ClientIP() in GIN 1.7 always return me 127.0.0.1

If I use
req.Header.Get("X-Forwarded-For")
I get the correct address too, like your example

I can't understand how to work with Nginx & GIN 1.7+

@artlevitan
Copy link
Author

Just in case, I post the source code to check

package main

import (
	"fmt"
	"net/http"
	"os"
	"time"

	"github.com/gin-gonic/gin"
)

// Response
type Response struct {
	Status  int    `json:"status"`
	Message string `json:"message,omitempty"`
}

// ResponseBody
type ResponseBody struct {
	Response   `json:"response"`
	Additional interface{} `json:"additional,omitempty"`
	Payload    interface{} `json:"payload,omitempty"`
}

// GinRouter - GIN-Router
func GinRouter() *gin.Engine {
	router := gin.New()

	// Set a lower memory limit for multipart forms
	router.MaxMultipartMemory = 100 << 20 // 100 MB

	// Custom Logger
	router.Use(gin.LoggerWithFormatter(func(param gin.LogFormatterParams) string {
		return fmt.Sprintf("%s |%s %d %s| %s |%s %s %s %s | %s | %s | %s\n",
			param.TimeStamp.Format(time.RFC1123),
			param.StatusCodeColor(),
			param.StatusCode,
			param.ResetColor(),
			param.ClientIP,
			param.MethodColor(),
			param.Method,
			param.ResetColor(),
			param.Path,
			param.Latency,
			param.Request.UserAgent(),
			param.ErrorMessage,
		)
	}))

	// Recovery middleware recovers from any panics and writes a 500 if there was one.
	router.Use(gin.Recovery())

	return router
}

func main() {

	// Server Settings
	router := GinRouter()
	server := &http.Server{
		Addr:           ":8080",
		Handler:        router,
		ReadTimeout:    30 * time.Second,
		WriteTimeout:   30 * time.Second,
		MaxHeaderBytes: 1 << 20,
	}

	// Ping test
	router.GET("/ping", func(c *gin.Context) {
		var respBody ResponseBody
		c.JSON(respBody.Status, respBody)
	})

	//
	err := server.ListenAndServe()
	if err != nil {
		fmt.Println("Could not start application")
		fmt.Println(err.Error())
		os.Exit(1)
	}
}

@leungyauming
Copy link

leungyauming commented Apr 29, 2021

Did you deploy your Nginx to the same machine as the server and the client?

Header X-Forwarded-For from Nginx in my case is correct too
But c.ClientIP() in GIN 1.7 always return me 127.0.0.1

If I use
req.Header.Get("X-Forwarded-For")
I get the correct address too, like your example

I can't understand how to work with Nginx & GIN 1.7+

I know you said that you can get the correct address through header.
But just want to make sure, because the $remote_addr variable will be 127.0.0.1 if you send a request to the Nginx proxy with the same machine.

@artlevitan
Copy link
Author

GO and Nginx are on the same server, everything was ok before the update gin 1.7

@leungyauming
Copy link

leungyauming commented Apr 29, 2021

Try to send a request to Nginx with another machine, because if you send the request using the same machine with Nginx, Nginx will set the $remote_addr variable of the proxy_set_header procedure to be 127.0.0.1 and then your Go application will receive 127.0.0.1 as the final IP of the "user".

@artlevitan
Copy link
Author

Are you proposing to change the server infrastructure, which works fine due to the fact that the GIN version has been updated?

@leungyauming
Copy link

leungyauming commented Apr 29, 2021

No, I didn't mean you need to change the server infrastructure. I just want to make sure that you are actually testing it in the right way.

  1. You said your Go application and your Nginx proxy server is deployed in the same machine
  2. You said getting the IP through the header is working but through the Nginx proxy is not working
  3. I'm not sure if you are using the same machine to send HTTP request to your Nginx proxy (because if you send a request to Nginx with the same machine your IP will be 127.0.0.1)

As the result, I think it may be caused by the 3 point I just mentioned. So I call you to try to "send request" to your Nginx proxy with a different machine, I didn't mean moving the Golang application to another machine.

@xpunch
Copy link

xpunch commented Apr 30, 2021

I meet the same problem too, after debug the application, I found this issue occur when engine.Run() was not called.
My application use gin as web handler, so it didn't need to startup by engine.Run, which makes trustedCIDRs not got initialized.
So it will never read IP from headers, case trustedCIDRs is nil.

@leungyauming
Copy link

I meet the same problem too, after debug the application, I found this issue occur when engine.Run() was not called.
My application use gin as web handler, so it didn't need to startup by engine.Run, which makes trustedCIDRs not got initialized.
So it will never read IP from headers, case trustedCIDRs is nil.

Good catch! I didn't actually notice that too.

@yiranzai
Copy link
Contributor

well, I pointed it out at the beginning. #2692

@leungyauming
Copy link

@artlevitan Please take a look and sorry for my misleading information.

@evanfuller
Copy link

I have also run into this problem. I see there is a PR to expose a workaround for setting trusted proxies, but I can't tell if it was abandoned.

@wackwinds
Copy link

when we start with RunTLS instand of Run, ClientIP() will always return 127.0.0.1 while prepareTrustedCIDRs() not called
it seems ClientIP just work fine under http, not https or others

@wackwinds
Copy link

看了下源码,prepareTrustedCIDRs只在run里有调用,RunTLS没调,也就是ClientIP只能在http环境使用,上面某评论说一直返回127.0.0.1其实也是因为走的https环境

@SteadyWorkerS
Copy link

nginx and gin v1.7 same machine, and send request to nginx from another machine, but method ClientIP return "127.0.0.1", which is not we want

@SennoYuki
Copy link

这个升级的破坏性真是神坑

@petanne
Copy link

petanne commented Jul 29, 2021

Bad and very destructive upgrade!!!

@montanaflynn
Copy link

montanaflynn commented Aug 4, 2021

@axiaoxin I had same problem, I used router.TrustedProxies = []string{"0:0:0:0:0:0:0:0001"} and it worked though.

@leungyauming When will the fix for code that doesn't use router.Run be released? This is basically all production code that uses graceful termination and is behind trusted proxies like cloudfront.

@montanaflynn
Copy link

By the way I ended up just using this function since I wanted the old logic and my servers are behind cloudfront:

func getClientIP(c *gin.Context) string {
	forwardHeader := c.Request.Header.Get("x-forwarded-for")
	firstAddress := strings.Split(forwardHeader, ",")[0]
	if net.ParseIP(strings.TrimSpace(firstAddress)) != nil {
		return firstAddress
	}
	return c.ClientIP()
}

Then replaced c.ClientIP() with getClientIP(c) and I'm back in business. Really unfortunate that just upgrading version caused us to lose days of user's real IP addresses, this should have been an opt-in change in my opinion, especially since it was broken.

@kainosnoema
Copy link

Also having this issue. We don't call engine.Run() and instead just use the handler directly. When will this get addressed?

@AlbinoGeek
Copy link

Also waiting for this to be fixed...

Without that method exposed, I can't enable the feature, as I don't start the server using .Run()

@dpanic
Copy link

dpanic commented Sep 1, 2021

By the way I ended up just using this function since I wanted the old logic and my servers are behind cloudfront:

func getClientIP(c *gin.Context) string {
	forwardHeader := c.Request.Header.Get("x-forwarded-for")
	firstAddress := strings.Split(forwardHeader, ",")[0]
	if net.ParseIP(strings.TrimSpace(firstAddress)) != nil {
		return firstAddress
	}
	return c.ClientIP()
}

Then replaced c.ClientIP() with getClientIP(c) and I'm back in business. Really unfortunate that just upgrading version caused us to lose days of user's real IP addresses, this should have been an opt-in change in my opinion, especially since it was broken.

I used your code, but modified it to be as a middleware which I invoke before all.

I did it this way in order to avoid wrapping and changing everywhere ctx.ClientIP()

func Preprocess(logger *zap.Logger) gin.HandlerFunc {
	return func(ctx *gin.Context) {

		_, port, _ := net.SplitHostPort(strings.TrimSpace(ctx.Request.RemoteAddr))

		ip := getClientIP(ctx)
		ctx.Request.RemoteAddr = fmt.Sprintf("%s:%s", ip, port)

		ctx.Next()
	}
}

Of course this is only mitigation, we're all waiting for regular fix of this issue.

@guillermo-menjivar
Copy link

any updates on this issue?

@adamwoolhether
Copy link

just discovered issues wtih this today as well....

@virskor
Copy link

virskor commented Oct 30, 2021

    proxy_set_header X-Real-IP   $remote_addr;
    proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
    proxy_set_header HTTP_X_FORWARDED_FOR $remote_addr;#在多级代理的情况下,记录每次代理之前的客户端真实ip
    proxy_set_header X-Forwarded-Proto $scheme;
    proxy_redirect off;

    
    proxy_pass https://remote_app_proxy_url;

after add NGINX configurations use code below to get real client IP

also Gin egine

router := gin.New()
router.TrustedProxies := []string{"0.0.0.0/0"}
router.RemoteIPHeaders :=  []string{"X-Forwarded-For", "X-Real-IP"}
func GetRealIP(c *gin.Context) string {
	ip := c.GetHeader("X-Real-IP")
	if len(ip) == 0 {
		return c.ClientIP()
	}

	return ip
}

will this comment help you? i hope that

@artlevitan
Copy link
Author

Hey! No, it didn't help me, I tried this method at the beginning.

@artlevitan
Copy link
Author

artlevitan commented Nov 2, 2021

I use this code until Gin is updated normally.

func clientIP(c *gin.Context) string {
	ip := c.GetHeader("X-Forwarded-For") // ip := c.ClientIP()

	// localhost
	if ip == "127.0.0.1" || ip == "::1" {
		return ""
	}
	// Drop Local / Reserved Address Ranges
	var (
		ip1_1 = net.ParseIP("10.0.0.0")
		ip1_2 = net.ParseIP("10.255.255.255")
		ip2_1 = net.ParseIP("127.0.0.0")
		ip2_2 = net.ParseIP("127.255.255.255")
		ip3_1 = net.ParseIP("169.254.0.0")
		ip3_2 = net.ParseIP("169.254.255.255")
		ip4_1 = net.ParseIP("172.16.0.0")
		ip4_2 = net.ParseIP("172.31.255.255")
		ip5_1 = net.ParseIP("192.168.0.0")
		ip5_2 = net.ParseIP("192.168.255.255")
	)
	trial := net.ParseIP(ip)
	if bytes.Compare(trial, ip1_1) >= 0 && bytes.Compare(trial, ip1_2) <= 0 {
		return ""
	}
	if bytes.Compare(trial, ip2_1) >= 0 && bytes.Compare(trial, ip2_2) <= 0 {
		return ""
	}
	if bytes.Compare(trial, ip3_1) >= 0 && bytes.Compare(trial, ip3_2) <= 0 {
		return ""
	}
	if bytes.Compare(trial, ip4_1) >= 0 && bytes.Compare(trial, ip4_2) <= 0 {
		return ""
	}
	if bytes.Compare(trial, ip5_1) >= 0 && bytes.Compare(trial, ip5_2) <= 0 {
		return ""
	}
	return ip
}

@thinkerou
Copy link
Member

v1.7.7 have released, thanks! https://github.com/gin-gonic/gin/releases

@bytejedi
Copy link

bytejedi commented Dec 3, 2021

Who cares about Trusted Proxies and Trusted shit... The bugs introduced by this feature are ridiculous. That's an env security stuff. This is the job of a load balancer or something like that in front of the gin server. Not suitable for enterprise users

@jiyeyuran
Copy link

Still not working if remote IP is ::1 .

@jiyeyuran
Copy link

	engine := gin.New()
	engine.SetTrustedProxies([]string{"0.0.0.0/0", "::/0"})

akijakya added a commit to banzaicloud/pipeline that referenced this issue Jan 24, 2022
Gin update to 1.7 broke the work with IP addresses of clients:
now 'X-Forwarded-For' and 'X-Real-Ip' headers needs to be set as
trusted for 'c.ClientIP()' method to successfully read the client's IP
in case 'gin.Run' method is not used. Related issue:
gin-gonic/gin#2697
@duaneking
Copy link

duaneking commented Sep 30, 2022

I just bumped into this; this "feature" is full of defects, imho.

@ECHibiki
Copy link

just set trusted proxies []string to have ::1 and get nginx to pass down the real ip...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests