moquette-io / moquette

Java MQTT lightweight broker
http://moquette-io.github.io/moquette/
Apache License 2.0
2.27k stars 814 forks source link

Connection filter interface for IP Bans #747

Open kazetsukaimiko opened 1 year ago

kazetsukaimiko commented 1 year ago

I'm doing an integration with an application server (Quarkus). Due to some legacy support requirements, we do need to leave some endpoints rather open to abuse, so we are monitoring client connections, message counts and payloads to watch for DoS attempts.

If their message count for a given topic far exceeds a normal threshold, or their payloads balloon in size, or they start sending us garbage, I'd like to ban the clients by IP. This is a shot at making an interface similar to the IAuthenticator interface for vetoing client connections.

Suggestions welcome. It is currently possible for me to work around the need for this using dependency injection, but that creates a circular dependency between the broker (Server::listConnectedClients) and the IAuthenticator.

kazetsukaimiko commented 1 year ago

Also, to the authors: It would also be helpful if I had a function to kick an active client off the broker somehow. I did not attempt that here. EDIT: See #744 , #748

source-c commented 1 year ago

Take a look at InterceptConnectMessage class - may be this is a more desired way to block particular client for your case? In this case you will need just to override the interceptor for your instance locally - there is no changes required on the broker side. Blocking entire IP is not always a good solution, you know.

kazetsukaimiko commented 1 year ago

Take a look at InterceptConnectMessage class - may be this is a more desired way to block particular client for your case? In this case you will need just to override the interceptor for your instance locally - there is no changes required on the broker side.

Thank you for your reply.

I have an InterceptHandler class implemented already. Please note, that in the Javadoc of the InterceptHandler class, it states: The events can act only as observers

All interceptor methods:

All InterceptConnectMessage methods return immutables or effectively immutable references. This is not a pattern for acting on events, merely their observation. This is useful, but not sufficient for proper access control, and the policing of misbehaving clients.

Blocking entire IP is not always a good solution, you know.

I am aware. An IP ban will be an escalation with a relatively short TTL. I am additionally seeking lighter means of action before this, such as kicking the client ID a few times, looking for things like many clients per IP that exhibit bad behavior, etc before even instituting an IP ban. But please understand that we have an internet-reachable broker up maybe a few months where we already see behaviors clearly identifiable as malicious brute force attempts and it just doesn't make sense to entertain any connections from these addresses for at least a few minutes/hours/days.

Additionally we are looking at updating our clients (IoT devices, the ones we can reach) to be more secure, but as mentioned cannot easily make changes server-side that will break the ability of legacy clients to connect. Those client rely on messaging to be notified of the updates we are trying to distribute.

Thanks again.

source-c commented 1 year ago

The events can act only as observers

Of course, this should be used to identify the problem, but not to eliminate it. I'm just trying to push the thoughts to the bit different direction - instead of relying on very low level of networking it might be desirable to identify a problem connection and, for example, to disconnect it and to lock its authorization.

We have accumulated a huge negative experience of banning by IP as a first shot - there are a lot of instances running on clouds or using dynamically allocated gateways to reach the broker. So, I just warned you to be honest. Never mind.

kazetsukaimiko commented 1 year ago

We have accumulated a huge negative experience of banning by IP as a first shot - there are a lot of instances running on clouds or using dynamically allocated gateways to reach the broker. So, I just warned you to be honest. Never mind.

Understandable. And its preferable to avoid banning good clients using legitimate services, VPNs, etc. I don't think its bad advice at all.

We do currently lack the ability to disconnect a client, something I noticed your PR does try to address. Thank you!

andsel commented 1 year ago

FYI #744 has been merged into the main

kazetsukaimiko commented 1 year ago

FYI #744 has been merged into the main

@andsel Thanks. Let me know if there's anything of value in this PR you'd like me to isolate, alternatively I can close this.

Also- any idea when a new release will roll out?

andsel commented 1 year ago

Hi @kazetsukaimiko first of all thank's for your interest in the project and sorry for late resp0nse.

Also- any idea when a new release will roll out?

We have some minor PRs such as #699 and #630, plus I have the idea to create a DSL to simplify the creation of the server configuration (instances of IConfig) to make embedding smoother.

Then I'll cut the 0.17 branch that at this point should be pretty stable, because I also want to move the next phase (MQTT5 support) into main and not keep for long the feature branch mqtt5_development.

Let me know if there's anything of value in this PR you'd like me to isolate,

I see value in this feature, have to check carefully the PR, because adding another argument to the startServer makes the method complicated. Maybe a builder class would help in this, positional arguments, when more than 3 and some are optional so implies a null makes the usage little bit harder.

kazetsukaimiko commented 1 year ago

I see value in this feature, have to check carefully the PR, because adding another argument to the startServer makes the method complicated.

Then perhaps solve the DSL and config builder problems first, and I'll resolve any conflicts after, would be my proposition.

Also, switch to Optional where appropriate.