mholt / caddy-ratelimit

HTTP rate limiting module for Caddy 2
Apache License 2.0
255 stars 17 forks source link

using with trusted_proxies / behind another proxy #19

Closed robgordon89 closed 1 week ago

robgordon89 commented 1 year ago

Thanks for your work on this, I am looking to implement this plugin to stop spam at the caddy level and rate limiting seems to be the best thing to do.

I am having an issue whereby the limits work but they will rate limit all requests with placeholder remote_host, I believe this is because its outside of the reverse_proxy handler and so trusted_proxies does not run before rate_limit

Is there a way to accomplish this?

mholt commented 1 year ago

I'm not sure I understand the question -- can you elaborate? And maybe share your config, the problem, etc?

robgordon89 commented 1 year ago

Hey, sorry let me try and explain better, I am trying to use this plugin to rate limit users to our landing pages, the issue I am having is that the rate limit is applied to everyone as we use caddy behind a proxy / CDN we get the wrong IP.

I am pretty sure it is related to https://github.com/caddyserver/caddy/issues/4924

Here is my rate limit config

rate_limit {
    distributed
    zone ip_rate {
        key    {remote_host}
        events {$CADDY_RATE_EVENTS:100}
        window {$CADDY_RATE_WINDOW:60s}
    }
}

The key here will be one of Cloudflare's IPs and so we rate limit multiple users.

Thanks Bob

mholt commented 1 year ago

Oh, yes I think that is related. You could also use the X-Forwarded-For header (if you trust that your clients are all coming from your CDN). @francislavoie might actually have more expertise with this.

francislavoie commented 1 year ago

I think once https://github.com/caddyserver/caddy/pull/5103 is merged, this plugin could inherit from that functionality to get the "real client IP".

For now, there's no "safe" way of doing it for this plugin on its own.

The caddyhttp package should probably provide an API for making this easy for plugins to add/implement (i.e. parsing/loading IP CIDRs for trusted proxies, and comparing the request against the trust list, pulling trust list from the server if necessary, etc).

robgordon89 commented 1 year ago

👋 hey @francislavoie so looks like we are on the last part of this and we will soon be able to use client_ip in the above rate limit is that correct?

https://github.com/caddyserver/caddy/pull/5104

rate_limit {
    distributed
    zone ip_rate {
        key    {client_ip}
        events {$CADDY_RATE_EVENTS:100}
        window {$CADDY_RATE_WINDOW:60s}
    }
}

Might see if I can build from this branch and see if i can get it working

francislavoie commented 1 year ago

That's right. There's no {client_ip} placeholder in that branch yet though I don't think. There's still some work to be done on it to make sure everything is correct. It's security sensitive so I need to make sure we get it right.

robgordon89 commented 1 year ago

cool, no worries it looks good to me anyways cant wait to test it out 👍

mholt commented 1 year ago

I believe this got merged in now if you want to try it (requires latest on master, as it's unreleased still).

teodorescuserban commented 1 week ago

Hey guys, why testing #68 I ran into this issue. I am using the rate limit module globally and trusted_proxies seemed to do nothing for the remote_host key. I have another caddy as a frontend and everything else seems to be working perfectly.

But the remote_host used as key for the rate limit seems to always be the internal IP of the frontend caddy.

Just changing the key from remote_host to client_ip seems to fix it. And of course since more than a year passed since the last comment, I most certainly have the https://github.com/caddyserver/caddy/pull/5104 included in my caddy.

I reckon this ticket can be closed. ;)

mholt commented 1 week ago

Awesome, thanks for sharing that!!