add connection rate limit per IP address (with custom hook)

[RobWei]

Hello,

I have implemented a feature that triggers a hook for exceeding a connection rate per IP address.
The value in connection_rate_limit_per_ip sets the time in seconds a client is not allowed to reconnect without triggering the hook.

The need for the implentation were clients that caused endless connection requests, one after another and caused maximum cpu consumption. On the other hand it could also be a targeted Denail of Service.

We will use the hook to block an IP address over iptables for 5 minutes. I have chosen the hook to allow for more flexibility in script design.

[RalfJung]

Thanks for the PR! I have seen high load due to repeated connection attempts as well in Freifunk Saar, so some rate limiting here might indeed make sense.

wants to merge 2 commits into wlanslovenija:newkernel

Please make your PRs against the master branch, not against feature branches.

[RalfJung]

Thanks a lot, this looks great! I like the approach, just have a few nits for the details.

@kaechele any opinion?

[RalfJung]

The PR is still marked as WIP, so I am not sure if you are even looking for a review already?

We will use the hook to block an IP address over iptables for 5 minutes. I have chosen the hook to allow for more flexibility in script design.

Could you add that as an example script?

[RalfJung]

@RobWei did you see my comment at #138 (comment)?

Also, this PR has merge conflicts with master.

And finally, should we be worried about accumulating too much data in that new dict you are adding? Is it worth having periodic cleanup, deleting all entries where the last connection attempt is more than TIME ago so it cannot be relevant any more?

[RalfJung]

@RobWei seems to be busy with other stuff and we are also seeing connection storms from single IPs in our network, so I'll likely push this PR to completion myself soon.

[RobWei]

@RalfJung
I am currently in my project work and still have 5 exam parts to pass.

I would be happy if someone can complete it, otherwise I would have to look at the Pull Request again in about one and a half months.

[RalfJung]

@RobWei understood, thanks for letting me know! I'll add this to my list.

Good luck with your exams. :)

[kaechele]

I am unsure whether we should use the IP and not the UUID as a key here.
Multiple clients could be behind the same IP address (think DS-Lite with CGNAT) and this change would punish regular clients if one client behind the same IP misbehaves.
I guess implementing this makes sense but the UUID as a key should also be implemented and the aforementioned caveat should be documented accordingly.

[RalfJung]

We can use the IP-UUID tuple, and see if that helps. If it does not we can always refine later.

[kaechele]

Actually I just re-read what this PR does and I find the description slightly misleading.
This merely provides a hook that's being called when a client exceeds the rate limit. No rate-limiting is actually being done here. Given that the logical reflex for many would be to push some iptables rules to drop packets from pesky clients this would go back to what I said above: punishing innocent clients since iptables does not evaluate the uuid.

Effectively this sounds more like a hack as it does not solve the actual problem, namely clients reconnecting too fast. Implementing some kind of back-off on the client side would be needed to fix the actual problem. And I have to say, in this regard the client is in fact buggy and should be fixed.

While I am not in favour of providing a band-aid when the actual problem could be solved at the root I can appreciate the fact that many broken clients are out in the field now and the likelyhood of them getting fixed anytime soon will rely heavily on autoupdate discipline in the respective communities.

I do agree that it is necessary though as we are seeing the same issues here.

[RalfJung]

I agree there is a bug in the client. However, I have no idea what is happening in the client to cause this. The client shouldn't connect to many brokers in a tight loop like these here do. Right now I don't even know where to start fixing this on the client.

[RalfJung]

@RobWei can you share the hook script that you are using to block clients, the one that sets up an iptables rule for 5 minutes?

[RalfJung]

I rebased this and updated it to take my comments into account. It is running fine on at least one of our servers (but without a hook configured).

But it would be nice to have an example hook as well.

[RalfJung]

Curious, CI is failing (and likely not spurious). Probably it triggers the rate limits.^^

[RalfJung]

This keeps getting weirder...

ERROR: Invalid signature for /tmp/tmp.WR47aol82I/index.asc

[RalfJung]

Thanks a lot @RobWei for the initial implementation. :)

FYI, I renamed the hook in the configuration, so if you switch to master you may have to update your config file.