moscajs / aedes

Barebone MQTT broker that can run on any stream server, the node way
MIT License
1.79k stars 231 forks source link

[feat] Mqttv5 CONNACK Return/Reason codes #783

Open braoutch opened 2 years ago

braoutch commented 2 years ago

Hello and thank you for this great broker,

I followed the documentation to implement my broker with authentication, rate limiting, and everything runs smoothly. However, When I close the connection to a client due to rate limits, they have no idea why their connection have cut. As far as they are concerned, it could be their internet connection, or my infrastructure could be down.

I think Reason Codes (MQTT 5) or Return Codes (MQTT 3) are made for that. I've read the MQTT 5 tickets that's mentioning features like that, but not with the current version. Is it possible to do something with the current version ?

You mention that the packet can be modified in the preConnect function. Is that a lead ? I tried everything, also with the callback, but it never reaches the client. Am I obliged to send a MQTT message to the client before cutting the connection ? That seems a lot of overhead to me.

Thank you for reading, I hope you'll have a idea for me :)

robertsLando commented 2 years ago

Unfortunately aedes doesn't support MQTT v5 yet, BTW reason codes are already supported by mqtt-packet. You could try to set those properties in the disconnect message

braoutch commented 2 years ago

Thx for the tip ! I'll check mqtt-packet. But where can I even find the disconnect message ? I don't think aedes sends such a message. As far as I know, the connection is cut either with callback(<Error>, false) or client.close() ? I can't see anywhere to put a disconnection message, or am I missing something ? I suppose a CONNACK packet that I could tune is emitted from somewhere, but I cannot find it (and neither can my paho client library)

robertsLando commented 2 years ago

Actaully you could set a custom returnCode property to the error in authenticate, it then will be parsed as shown here: https://github.com/moscajs/aedes/blob/e3aa21499bbfc1b42dc71c5925a40248a45c0827/lib/handlers/connect.js#L120

But as you can see errors are a bit limited there, mqtt v5 added a new flag named reasonCode to connack with much more errors: https://docs.oasis-open.org/mqtt/mqtt/v5.0/os/mqtt-v5.0-os.html#_Toc3901079

It also allow to specify a reason string in connack properties (along with lot others props). The solution here would be to implement such feature on aedes itself, at least on connack negate function add the support to specify a reasonCode and a reasonString to be added in the connack packet when protocol il 5.

Would you like to submit a PR?

braoutch commented 2 years ago

Thank you again for this precise response. Reading aedes code where you told me, I understood that the error code is only sent to the client in the connack after an authenticate method, not a preConnect. Weirdly, I could only get -1 or 2 error codes, but that will be sufficient for now. I don't think I'm skilled enough to make a PR for MQTT5 compatibility :)

braoutch commented 2 years ago

In my free time I'll check if I can do something about a "partial" mqtt 5 compatibility, meaning just the reason codes.

robertsLando commented 2 years ago

Keep this open for now, there could be a reference for other users that want to implement such feature