$ prefix topic subscriptions are invalid and a session is disconnected

[gechelberger]

Expected Behavior

A subscription should be registered for any messages sent to a topic with a $ prefix.

Section 4.7.2 of both v3.1.1 and v5:

The Server MUST NOT match Topic Filters starting with a wildcard character (# or +) with Topic Names 
beginning with a $ character [MQTT-4.7.2-1]. The Server SHOULD prevent Clients from using such 
Topic Names to exchange messages with other Clients. Server implementations MAY use Topic Names 
that start with a leading $ character for other purposes. 

It seems like it may be ok to block messages published to such a topic but subscriptions should be allowed.

Current Behavior

The subscription is refused and the connection terminated.

Failure Logs

mqttplay-broker-1  |   2023-06-23T22:39:22.186826Z  WARN rumqttd::router::routing: Subscription cannot be validated: Invalid filter prefix $SYS/#, reason: InvalidFilterPrefix("$SYS/#")
mqttplay-broker-1  |     in rumqttd::router::routing::incoming_payload with client_id: "mqtt-explorer-ad980b3f"
mqttplay-broker-1  |     in rumqttd::router::routing::[>] incoming with connection_id: 1
mqttplay-broker-1  | 
mqttplay-broker-1  |   2023-06-23T22:39:22.186908Z ERROR rumqttd::server::broker: router-drop, error: Recv(Disconnected)
mqttplay-broker-1  |     in rumqttd::server::broker::remote_link with tenant_id: None, client_id: "mqtt-explorer-ad980b3f", connection_id: 1

[swanandx]

Hey, One use of $ is to have shared subscription, like $share/group/topic in v5. This will be added by #628 PR.

Other than that, I can't think of a reason why client should be able to subscribe to $topic.

Docs also say:

Applications cannot use a topic with a leading $ character for their own purposes

Currently, $ isn't used for any special purpose by the Server, so I don't think it matters? Or do you want to say, let Client register the subscription, disconnect only those who try to publish to such topic names?

[swanandx]

The Server SHOULD prevent Clients from using such Topic Names to exchange messages with other Clients

It's also mentioned that we should prevent use of such topic names. So it should be ideal to disconnect in case someone tried to use such topic name even for subscription right?

[gechelberger]

I think it is acceptable to disconnect clients or drop packets should clients publish to a $ prefix topic, but we should accept subscriptions even if it is just a NOOP for the broker.

For example, MQTT Explorer is a commonly used utility which, on connection, tries to subscribe to the topics under "$SYS/#", expecting metrics as provided by mosquito and other brokers.

Non-normative comment
    * $SYS/ has been widely adopted as a prefix to topics that contain Server-specific information or control APIs
    * Applications cannot use a topic with a leading $ character for their own purposes

Non-normative comment
    * A subscription to “#” will not receive any messages published to a topic beginning with a $
    * A subscription to “+/monitor/Clients” will not receive any messages published to “$SYS/monitor/Clients”
    * A subscription to “$SYS/#” will receive messages published to topics beginning with “$SYS/”
    * A subscription to “$SYS/monitor/+” will receive messages published to “$SYS/monitor/Clients”
    * For a Client to receive messages from topics that begin with $SYS/ and from topics that don’t begin with a $, it has to subscribe to both “#” and “$SYS/#”

These are common conventions across the mqtt ecosystem and we shouldn't be breaking compatibility for other clients.

[swanandx]

We can allow subscriptions to $ like NOOP ( unless they are $share or something our server publishes to ). Thank you for clarifying!

validate_subscription() functions need to be updated to allow filter starting with $, but we have to validate it further. Other than that, we also have to update the matches ( which uses this ) such that it adheres to specifications.

Let me know if you would like to contribute!

[gechelberger]

I'm happy to take a crack at it.

Handling the subscription and filter matching side seems pretty strait forward.

Validating the incoming published messages seems a little bit more involved. I also noticed while I was poking around that there appears to be a bug in handle_device_payload for published messages where the topic/alias gets normalized on a clone of the Publish struct in append_to_commitlog but then the original Publish gets passed to the MeterData which will use the unaliased/empty topic and misattribute some statistics.

It seems like we would want to validate the explicit topic before looking at aliasing to make sure that we don't store an invalid topic as an alias and then normalize the topic/alias before handing it off to append_to_commitlog and register_publish. Since we call append_to_commitlog from a few different places I'm not sure exactly how those all interact and what invariants need to be enforced there.

[swanandx]

Good catch! I totally missed the publish part haha.

For MeterData part, can you open a new issue? just so it's discussion will be easier to find by others. Thanks 😊

[gechelberger]

Can do