redboltz / mqtt_cpp

Boost Software License 1.0
434 stars 107 forks source link

Client keep_alive must be non-zero to connect to default mosquitto 2.0.13 #904

Open DavidAntliff opened 2 years ago

DavidAntliff commented 2 years ago

Hi, I've been happily using this library with mosquitto 2.0.11 for a few months. I've recently attempted to update to mosquitto 2.0.12 & 2.0.13 and run into a connection problem. The server is rejecting client connections with error 2 (identifier_rejected).

I'm using commit 29f93bc0c0cb82979840318e49fcce710f07ec88 (and was previously using 70e45c258c4b20f2d219533add5afd7693c1da39, which has the same issue).

Mosquitto broker log:

mosquitto  | 1636940175: New client connected from 172.18.0.1:42770 as MyMQTTClient (p2, c1, k0).
mosquitto  | 1636940175: No will message specified.
mosquitto  | 1636939846: Sending CONNACK to MyMQTTClient (0, 2)
mosquitto  | 1636939846: Bad socket read/write on client MyMQTTClient: Invalid arguments provided.

In my client->set_connack_handler(bool sp, mqtt::connect_return_code connack_return_code) callback I'm getting sp is false, and connack_return_code is 2, or identifier_rejected.

I don't personally understand the exact issue, so I'm bringing it to your attention because some other client implementations have had the same issue lately, and you might better understand the implications:

telegraf: https://community.influxdata.com/t/get-identifier-rejected-error-while-using-outputs-mqtt-mosquitto-version-is-2-0-12/21702 micropython: https://forum.micropython.org/viewtopic.php?t=11076&p=60836

Perhaps this is also relevant? A breaking change in mosquitto for a zero keepalive value sent by a client? https://github.com/eclipse/mosquitto/issues/2117

For now I can remain with mosquitto 2.0.11 but if you'd like me to test any changes against 2.0.12 or 2.0.13 I can do this fairly easily at the moment.

DavidAntliff commented 2 years ago

I can confirm that the issue is with the default value of the client's keep_alive.

By setting this to a non-zero value, mqtt_cpp is able to connect to mosquitto 2.0.12 and 2.0.13.

    client->set_keep_alive_sec(1);

What I'm not sure about is whether this is intended behaviour by mosquitto, or a bug. I've asked for clarification from the mosquitto project.

EDIT: turns out this is intended behaviour, and you must set max_keepalive to 0 in the mosquitto config if you want a client with keep_alive set to 0 to be able to connect. Note that 2.0.12 is broken, use 2.0.13 instead.

DavidAntliff commented 2 years ago

I've worked out what's going on and my summary is here.

Since it would break existing projects to change the default keepalive in mqtt_cpp, you might want to consider advising in your README for users to change the default value of keep_alive to non-zero if they want it to work out-of-the-box with mosquitto 2.0.13 or newer (or change the mosquitto config), otherwise many of your new users who try to use it with mosquitto are going to run into this problem quickly and potentially move onto other libraries when it doesn't work.

kleunen commented 2 years ago

But is this going to be fixed in mosquitto ? Because this does sound a bit like this is a mosquitto issue. I think a default of no keep alive for a client is quite reasonable.

DavidAntliff commented 2 years ago

@kleunen I tried to get an answer to that back in November but failed (see my link above) - from what I can tell it's an intentional change in mosquitto to prohibit no-keep-alive clients by default. Maybe there was a security incident behind it, I really don't know. I reported this here because this great library doesn't work out-of-the-box with mosquitto any more and it's a non-trivial exercise to determine why. Maybe @ralight can advise?

kleunen commented 2 years ago

Yes it would be good to understand the reason behind why mosquitto is rejecting these clients. I am not saying at this moment we should not add this to the documentation of mqtt_cpp. But at least this behaviour should be documented in the mosquitto documentation. Because not only mqtt_cpp is effected but any client having no keep alive (by default).