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

[Cloudflare] real IP ".ClientIP()" in logs #3336

Closed
the-hotmann opened this issue Sep 24, 2022 · 9 comments
Closed

[Cloudflare] real IP ".ClientIP()" in logs #3336

the-hotmann opened this issue Sep 24, 2022 · 9 comments

Comments

@the-hotmann
Copy link

the-hotmann commented Sep 24, 2022

My setup is like this:

Client ==:> Cloudflare ==:> NPM ==:> Docker_Container ==:> gin

in my logs I ofc just see Cloudflare IPs like:

  • 162.158.90.247
  • 172.70.250.47
  • 172.70.250.251
  • 172.70.251.126
  • 172.70.250.37
  • 172.70.251.126
  • 172.70.250.47

I know that I can restore the real IPs in the app, but how do I get Gin to display the real IPs in the log?
Is there a handy module I can use or any parameter that needs to be set, to make it display real IPs, instead of Cloudflare ones?
If not, that would be a very cool feature - having a "switchable" Cloudflare-Integration.
Maybe something like:

gin.SetMode(gin.CloudflareMode)

The IP ranges (IPv4, IPv6) ofc should be updated automatically before start, so that it is always up to date. But if gin, shipps with the current IP ranges as hard-fallback, that would be even better. But every release should have built-in the newest IP ranges of Cloudflare. Cloudflare's IP ranges not often change, so shipping built-in hard-fallbacks should be pretty stable, and if possible (IPv4 & IPv6 are reachable) it anyway gets updated.

I think that makes a lot of sense, as many people are using Cloudflare as a proxy. Turning on CloudflareMode should set .ClientIP() to the real IP and also log the real IP, so that no changes need to be done in the code.

Thanks in advance!

@duaneking
Copy link

duaneking commented Oct 1, 2022

Please close this bug if this link answers your issue.

https://support.cloudflare.com/hc/en-us/articles/206776727-Understanding-the-True-Client-IP-Header

@the-hotmann
Copy link
Author

the-hotmann commented Oct 1, 2022

I am very well aware of, what the True-Client-IP-Header is - thanks.

But, this does not make my Gin logs show it. That is why I wanted to bring this topic up to talk about better Cloudflare-Integration and maybe even a very easy toggle (a CloudflareMode) which then automatically also shows the real IPs in the log.

This actually shall not be seen as a bug, but more like a discussion, if you guys don't mind.

Anyway I would love to know a nice way to replace all IPs with the real IPs in the gin logs. For me this would solve the problem I have atm, but like I said, thinking forward, a mode that does all this by just setting the right config would be awesome and I guess people will appreciate it, as many people use Cloudflare and it would make gin more attractive.

The question was:

I know that I can restore the real IPs in the app, but how do I get Gin to display the real IPs in the log?
Is there a handy module I can use or any parameter that needs to be set, to make it display real IPs, instead of Cloudflare ones?

So no, this does not answer my question at all - sorry.

@pscheid92
Copy link

pscheid92 commented Oct 8, 2022

I think there is a more or less our of box solution for your need.


Gin retrieves the IP address from c.ClientIP(), even for the log output.

ClientIP implements one 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 is it will then try to parse the headers defined in Engine.RemoteIPHeaders (defaulting to [X-Forwarded-For, X-Real-Ip]). If the headers are not syntactically valid OR the remote IP does not correspond to a trusted proxy, the remote IP (coming from Request.RemoteAddr) is returned.

Source: Documentation for Context.ClientIP

// RemoteIPHeaders 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 defined by `(*gin.Engine).SetTrustedProxies()`.
RemoteIPHeaders [][string](https://pkg.go.dev/builtin#string)
// ForwardedByClientIP 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](https://pkg.go.dev/builtin#bool)

So by adding Cloudflare's True-Client-IP to Engine.RemoteIPHeaders and activating ForwardedByClientIP gin should be able to resolve the true client IP by calling Context.ClientIP() and log it.


Server Code

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

	r.RemoteIPHeaders = append(r.RemoteIPHeaders, "True-Client-IP")
	r.ForwardedByClientIP = true

	r.GET("/ip", func(c *gin.Context) {
		ip := c.ClientIP()
		c.String(200, ip)
	})

	if err := r.Run(); err != nil {
		log.Fatalln(err)
	}
}

Client Calls

# Without True-Client-IP Header
$ curl localhost:8080/ip     
::1

# With True-Client-IP Header
$ curl localhost:8080/ip -H "True-Client-IP: 123.123.123.123"
123.123.123.123

Server Logs

go run . 

# ... snip ...

[GIN-debug] Listening and serving HTTP on :8080
[GIN] 2022/10/08 - 15:26:39 | 200 |      37.413µs |             ::1 | GET      "/ip"
[GIN] 2022/10/08 - 15:27:29 | 200 |      35.438µs | 123.123.123.123 | GET      "/ip"

Note:
As mentioned in the documentation, the proxy must be trusted. The default configuration trusts every proxy but logs a warning. Cloudflare provides documentation and even an API to fetch all IP ranges. One could use that to init the trusted proxies correctly.

https://www.cloudflare.com/en-gb/ips/

@duaneking
Copy link

duaneking commented Oct 9, 2022

Respectfully, Please, No.

Respectfully, If I understand your request, you want gin to support a hosting specific "easy mode" that keeps you from needing to learn the common web standards, and you want it for only Cloudflare even though most people using this library are not using Cloudflare.

This is not a Cloudflare specific library; This is not a hosting provider specific library. It must not turn into one or it will die.

Due to this, I'm against this library being focused on supporting only Cloudflare as its main objective, or having features specific to it that Host that are not useful on other hosts like Azure or AWS; Its also clear that Gin having only Cloudflare support is not its objective, so I'm against it being coopted or limited in that way as it currently works fine on GCP, Azure, and AWS in my tests, and I want that support to stay, because both myself and others I know are using it successfully on these platforms in production right now.

IMHO. This is a bad request.

To be clear: I have no problem with it supporting Cloudflare as well as it supports other hosts lie AWS, Azure, GCP, etc, but I don't believe Gin should be build to focus on one specific hosting provider at all, it should be built to support web standards and then have support for these different hosts via that web standard support, not pull in the limitations or hosting specific things that people not using these hosts wont need, but will still be burdened by.

For example, every line of dead code added to support only Cloudflare means that's dead code we need to deal with for Lambda Deployments on AWS or Azure FaaS; and that's size limited so every byte counts. It would be much more SOLID and DRY code to simply support the web standards and have examples for each provided by the community; I'm 100% against adding Cloudflare-only code as it will just be bloat for anybody not using that provider. Same with AWS specific code, same with Azure specific code, etc.

And if it gets to that, I'm going to be forced to fork Gin just to keep things running, because I use it in Azure and AWS only myself.

Your use case is similar to one others have on other hosts, but the implantation and header usage will always be different for each host; as an example on AWS Lambda you set the clients Remote-IP using "X-Forwarded-For" as api gateways will send that header and not the customer one CF uses just for marketing and branding to be different:

 	router := gin.New() // gin.Default()

	router.SetTrustedProxies(nil)

	router.TrustedPlatform = "X-Forwarded-For"

IMHO We should NOT be trying to support specific vendors or hosts; that brings in technical debt and dead code that others will be bound by but not actually use. Instead, you should learn the web standards, and if needed, provide hosting provider specific examples to the community via the contrib, instead of forcing them as part of core gin.

I get that you want a "easy mode" for your provider; but this is not the right place for it from an architectural and system design perspective. Your asking to build a room only for that Cloudflare provider in a home shared by multiple, thats why other attempts at this were depreciated.

@pscheid92
Copy link

@duaneking Thank you for pointing out TrustedPlatform! I didn't know about it yet 😄 But the source code and Documentation: Don't Trust All Proxies helped me 😇

@the-hotmann
Copy link
Author

Respectfully, Please, No.

I now, after reading your post understand.

router.TrustedPlatform = "X-Forwarded-For"

That was, what I originally was searching for. Even tho I asked for a specific Cloudflare integration, this was what I actually wanted. Makes it way more flexible and easy for all.

Thank you also to @pscheid92 !

The problem is solved!

@duaneking
Copy link

Glad I could help.

I wrote it this way for others as I have found that closed bugs are a great resource for common Gin issues.

@Bisstocuz
Copy link
Contributor

@duaneking Thank you for pointing out TrustedPlatform! I didn't know about it yet 😄 But the source code and Documentation: Don't Trust All Proxies helped me 😇

🤣 I submited this in #2906, I specifically added the documentation in case people didn't know, but apparently you didn't read it carefully.

@GwynethLlewelyn
Copy link

... and here was I stupidly adding a very trivial middleware just to retrieve the 'correct' IP address from the Cloudflare headers...

Silly me!...

I wrote it this way for others as I have found that closed bugs are a great resource for common Gin issues.

@duaneking you couldn't have done a better job. And here am I, proving you correct!

Note that the middleware solution is overkill, but it can always be used for other things (such as consistently getting the country ID that Cloudflare sends, as opposed to doing a local lookup on the passed 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

5 participants