GetIP TrustForwardHeader varies per request about limiter HOT 7 CLOSED

You can use this version: https://github.com/ulule/limiter/releases/tag/v3.1.0

Cheers,

from limiter.

Hello,

Sorry in advance but I'm not sure to understand the issue here... So, I'll try to respond (and explain) as much as I can.

Since the limiter object is instantiated early before the server stats and then shared among different requests whose IPs are different.

Yeah that's the purpose.
Either your application is publicly facing the network (so it has a public IP on the internet) and you shouldn't rely on X-Real-IP and X-Forwarded-For headers because, someone could spoof (or forge) an IP address to manipulate your limiter counters.
Or, you have a reverse proxy (or a load balancer, a frontend, etc...) that will forward public request from the internet into the private network of your application, so you have to trust X-Real-IP and X-Forwarded-For headers because otherwise, you'll limit your reverse proxy and not the users. Doing both is an anti pattern in my opinion, you shouldn't do that.

IMHO the TrustForwardHeader should vary based on how much the server trusts the IP and should not be as a config rule.

With this previous explanation, you shouldn't. It should be a config rule because you know if your application will be deployed on a public network, or a private one.

However, it's possible that I've completely missed your point. In such case, could you provide an example of your network topology and/or an example so I could understand.

Also the change of GetIP is not backward compatible - AKA broke my build :)

Yeah, that's why we released a new major version v3.0.0 that indicate a compatibility break.
If you're not familiar with the Semantic Versioning approach, you can read this website: https://semver.org/
Or fallback to an older version, like v2.2.2.

Regards,

from limiter.

Thanks for the detailed reply. You understood the question perfectly.

Or, you have a reverse proxy (or a load balancer, a frontend, etc...) that will forward public request from the internet into the private network of your application, so you have to trust X-Real-IP and X-Forwarded-For headers because otherwise, you'll limit your reverse proxy and not the users. Doing both is an anti pattern in my opinion, you shouldn't do that.

I think this should be left to the developer to decide on how the server is being setup. On a small scale you could lock your server behind a load balancer or varnish, but in practice, if your app is on Kubernetes for ex then you would need to dynamically request new set of IPs to update your rate limiter. And you probably don't want to restart your node every time you are doing that.

Also remember that the old GetIP() gave the option to the developer to manage it internally, either via a config or conditionally, but the new function does not.

from limiter.

I think this should be left to the developer to decide on how the server is being setup.

Yeah, you can do that by using WithTrustForwardHeader function, that create an Option for limiter.New, so you can decide if you want to trust X-Real-IP and X-Forwarded-For headers. Is that what you're looking for ?

Nevertheless, you can create a custom middleware if the current implementation doesn't suit you.

Just change middleware.Limiter.GetIPKey(r) by a custom handler that can dynamically accept or refuse these headers from your dynamic configuration.

Cheers,

from limiter.

What I meant is that the proxy IPs will change over time and the server should take care of it dynamically. Please take a look at https://gist.github.com/gadelkareem/5a087bfda1f673241d0ac65759156cfd the cloudflare IPs are subject to change over time and you will probably need to update it without the need to restart the server.

from limiter.

Can you look at this new function and tell me if you'll use it?
https://github.com/ulule/limiter/blob/feat/network-functions/network.go#L34

Otherwise, I won't merge this into upstream.
Thanks.

from limiter.

Looks good!

from limiter.