Connection filter interface for IP Bans #747

kazetsukaimiko · 2023-05-01T16:39:55Z

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 · 2023-05-01T16:43:21Z

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 · 2023-05-03T09:30:14Z

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 · 2023-05-03T09:54:35Z

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:

Return void
Do not define what behavior is when throwing an exception.

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 · 2023-05-03T10:53:45Z

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 · 2023-05-03T13:36:11Z

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 · 2023-06-03T19:52:32Z

FYI #744 has been merged into the main

kazetsukaimiko · 2023-06-03T22:13:50Z

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 · 2023-06-10T15:28:50Z

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 · 2023-06-10T18:38:18Z

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.

andsel

I've left some comments.
I think we can annotate the new interface also as FunctionInterface, so that in some cases, maybe some test we can leverage some goodies provided by Java 8+

andsel · 2023-06-16T13:12:51Z

broker/src/main/java/io/moquette/broker/Server.java

@@ -275,6 +277,7 @@ public void startServer(IConfig config, List<? extends InterceptHandler> handler
        initialized = true;
    }

+


Suggested change

andsel · 2023-06-16T13:13:53Z

broker/src/main/java/io/moquette/broker/Server.java

@@ -472,6 +475,16 @@ private IAuthorizatorPolicy initializeAuthorizatorPolicy(IAuthorizatorPolicy aut
        return authorizatorPolicy;
    }

+    private IConnectionFilter initializeConnectionFilter(IConnectionFilter connectionFilter, IConfig props) {
+        LOG.debug("Configuring MQTT Connection Filter");


The Debug log should carry some useful information like, the connectionFilterClassName

andsel · 2023-06-16T13:18:09Z

broker/src/main/java/io/moquette/broker/Server.java

+        String connectionFilterClassName = props.getProperty(BrokerConstants.CONNECTION_FILTER_CLASS_NAME, "");
+
+        if (connectionFilter == null && !connectionFilterClassName.isEmpty()) {
+            connectionFilter = loadClass(connectionFilterClassName, IConnectionFilter.class, IConfig.class, props);
+        }
+        return connectionFilter;
+    }
+


This code is a copy and paste of the initial part of initializeAuthenticator and initializeAuthorizatorPolicy, so maybe could be extracted in a separate method and reused.

andsel · 2023-06-16T13:27:30Z

broker/src/test/java/io/moquette/broker/MQTTConnectionPublishTest.java

@@ -94,7 +96,7 @@ private MQTTConnection createMQTTConnection(BrokerConfiguration config, Channel
        sessionRegistry = new SessionRegistry(subscriptions, memorySessionsRepository(), queueRepository, permitAll, scheduler, loopsGroup);
        final PostOffice postOffice = new PostOffice(subscriptions,
            new MemoryRetainedRepository(), sessionRegistry, ConnectionTestUtils.NO_OBSERVERS_INTERCEPTOR, permitAll, loopsGroup);
-        return new MQTTConnection(channel, config, mockAuthenticator, sessionRegistry, postOffice);
+        return new MQTTConnection(channel, config, mockAuthenticator, connectionFilter, sessionRegistry, postOffice);


Given that in this test we don't use the filter we could inline it, without creating an used variable:

return new MQTTConnection(channel, config, mockAuthenticator, new MockConnectionFilter(), sessionRegistry, postOffice);

Or better, given that it's a pass-all filter, a more explicit implementation could be used for such cases:

public class AcceptAllFilter implements IConnectionFilter { @Override public boolean allowConnection(ClientDescriptor clientDescriptor) { return true; } }

In all test places where special filtering logic is not needed we can use:

return new MQTTConnection(channel, config, mockAuthenticator, new AcceptAllFilter(), sessionRegistry, postOffice);

This comment is valid also for other places where the MockConnectionFilter is used with the same intention.

Connection filter interface for IP Bans

b36af55

kazetsukaimiko mentioned this pull request May 1, 2023

Attempt at a method to kick an active client. #748

Closed

source-c mentioned this pull request May 20, 2023

Feature disconnects #744

Merged

Merge branch 'main' into connection_filters

b1697d0

andsel requested changes Jun 16, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Connection filter interface for IP Bans #747

Connection filter interface for IP Bans #747

kazetsukaimiko commented May 1, 2023

kazetsukaimiko commented May 1, 2023 •

edited

source-c commented May 3, 2023

kazetsukaimiko commented May 3, 2023 •

edited

source-c commented May 3, 2023 •

edited

kazetsukaimiko commented May 3, 2023

andsel commented Jun 3, 2023

kazetsukaimiko commented Jun 3, 2023 •

edited

andsel commented Jun 10, 2023

kazetsukaimiko commented Jun 10, 2023

andsel left a comment

andsel Jun 16, 2023

andsel Jun 16, 2023

andsel Jun 16, 2023

andsel Jun 16, 2023

		@@ -275,6 +277,7 @@ public void startServer(IConfig config, List<? extends InterceptHandler> handler
		initialized = true;
		}

Connection filter interface for IP Bans #747

Are you sure you want to change the base?

Connection filter interface for IP Bans #747

Conversation

kazetsukaimiko commented May 1, 2023

kazetsukaimiko commented May 1, 2023 • edited

source-c commented May 3, 2023

kazetsukaimiko commented May 3, 2023 • edited

source-c commented May 3, 2023 • edited

kazetsukaimiko commented May 3, 2023

andsel commented Jun 3, 2023

kazetsukaimiko commented Jun 3, 2023 • edited

andsel commented Jun 10, 2023

kazetsukaimiko commented Jun 10, 2023

andsel left a comment

Choose a reason for hiding this comment

andsel Jun 16, 2023

Choose a reason for hiding this comment

andsel Jun 16, 2023

Choose a reason for hiding this comment

andsel Jun 16, 2023

Choose a reason for hiding this comment

andsel Jun 16, 2023

Choose a reason for hiding this comment

kazetsukaimiko commented May 1, 2023 •

edited

kazetsukaimiko commented May 3, 2023 •

edited

source-c commented May 3, 2023 •

edited

kazetsukaimiko commented Jun 3, 2023 •

edited