ESPWortuhr / Multilayout-ESP-Wordclock

Software for an ESP8266 based word clock with support of different layouts and languages
86 stars 46 forks source link

Anonymous MQTT not working #355

Closed J4nsen closed 4 months ago

J4nsen commented 8 months ago

Hi, my mqtt-server (mosquitto) only allows anonymous connections or ones for existing users.

However, ESPWortuhr always seems to set the user and password to 29 empty spaces, when the fields in the web ui are blank. image

It would be nice, if a connection without username and password would be made, when the fields are empty, i.e., setting username and password to NULL on the connect-function.

Thanks for the nice project! :)

dbambus commented 8 months ago

Hey @J4nsen,

correct me if I m wrong, but the 20 fields in Hex are supposed to be NULLs not blank space ( 40). In the payload handler of the Website include/WebPageAdapter.h the code is checking in the Payload for spaces in reverse order.

    for (int8_t counter = PAYLOAD_LENGTH - 2; counter > -1; counter--) {
        if (!isSpace(text[counter])) {
            index = counter + 1;
            break;
        }
    }

So the username and the password should be empty.

What could be done, adding a check for empty mqtt.user/mqtt.password in the include/mqtt.h and hand the function parameters over as NULL, as described in the doc of the MQTT client.

boolean PubSubClient::connect(const char *id) {
    return connect(id,NULL,NULL,0,0,0,0,1);
}

boolean PubSubClient::connect(const char *id, const char *user, const char *pass) {
    return connect(id,user,pass,0,0,0,0,1);
}

Could you try that ?

Cheers David

dbambus commented 8 months ago

And, what build are you using ? Are you on the most recent git state ?

J4nsen commented 8 months ago

Hi @dbambus, thanks for the fast reply.

correct me if I m wrong, but the 20 fields in Hex are supposed to be NULLs not blank space ( 40). In the payload handler of the Website include/WebPageAdapter.h the code is checking in the Payload for spaces in reverse order.

Wireshark is showing hex values, 20 is a space, 40 would be an @-char.

    for (int8_t counter = PAYLOAD_LENGTH - 2; counter > -1; counter--) {
        if (!isSpace(text[counter])) {
            index = counter + 1;
            break;
        }
    }

So the username and the password should be empty.

I dont completely understand the code (yet). What I see is that when opening the web-ui there are whitespaces in the username and password field. Even after removing them and saving the settings they reappear after reloading the ui.

What could be done, adding a check for empty mqtt.user/mqtt.password in the include/mqtt.h and hand the function parameters over as NULL, as described in the doc of the MQTT client.

boolean PubSubClient::connect(const char *id) {
    return connect(id,NULL,NULL,0,0,0,0,1);
}

boolean PubSubClient::connect(const char *id, const char *user, const char *pass) {
    return connect(id,user,pass,0,0,0,0,1);
}

Could you try that ?

I've applied the following patch. Wordclock is now able to connect to the broker :)

diff --git a/include/mqtt.hpp b/include/mqtt.hpp
index d5ac0b2..a556552 100644
--- a/include/mqtt.hpp
+++ b/include/mqtt.hpp
@@ -16,14 +16,14 @@ PubSubClient mqttClient(client);
 void Mqtt::init() {
     mqttClient.setServer(G.mqtt.serverAdress, G.mqtt.port);
     mqttClient.setCallback(callback);
-    mqttClient.connect(G.mqtt.clientId, G.mqtt.user, G.mqtt.password);
+    mqttClient.connect(G.mqtt.clientId);
     mqttClient.subscribe((std::string(G.mqtt.topic) + "/cmd").c_str());
 }

 //------------------------------------------------------------------------------

 void Mqtt::reInit() {
-    mqttClient.connect(G.mqtt.clientId, G.mqtt.user, G.mqtt.password);
+    mqttClient.connect(G.mqtt.clientId);
     reconnect();
 }

However, the connection seems a bit unstable. I see error messages like these in my broker log:

1704464320: Client Wordclock has exceeded timeout, disconnecting.
[...]
1704464588: New client connected from 192.168.123.123:51867 as Wordclock (p2, c1, k15).
1704464606: Client Wordclock disconnected due to malformed packet.

And, what build are you using ? Are you on the most recent git state ?

I'm on the latest git commit. I could write a PR which checks for an whitespace-only username/password and then uses the connect function without credentails. However, I think there is another problem, e.g., why am I unable to clear the whitespaces in the webui?

dbambus commented 8 months ago

Hey,

I will elaborate the whitespaces once I finished work for today and also I ll try Wireshark and take a look at the mosquitto logs for it. Never had that issue before. Maybe try another QoS Level ? See PubSubClient.cpp for more Doc.

Just a question, do you have your mosquitto running in a Docker ? Maybe take could also be an indicator: https://stackoverflow.com/questions/75534407/mqtt-malformed-packet

J4nsen commented 8 months ago

Hi @dbambus, no hurry :)

Just a question, do you have your mosquitto running in a Docker ? Maybe take could also be an indicator: https://stackoverflow.com/questions/75534407/mqtt-malformed-packet

mosquitto is running directly on the host. The reason why some clients cannot connect is that im running the dynamic-security plugin with allow_anonymous = true, which basically rejects mqtt-clients with a username, if they are not listed in the local ACLs.

This problem isn't new to me. For example, for Tasmota clients I have to disable/clear the username/password with "MqttUser 0" and "MqttPassword 0" or else it will use the firmware default.

dbambus commented 4 months ago

Hey @J4nsen,

i uploaded an update to the software to handle anonymous logins to the Matt broker, with https://github.com/ESPWortuhr/Multilayout-ESP-Wordclock/pull/396. Hereby I close this issue to keep an overview. Really thank you for this issue :-), maybe you ll find some more improvements for the software. David

J4nsen commented 4 months ago

Hey @dbambus, awesome! :)

I had a quick look at the code and saw that the reInit-function could potentially be called with user and password and thus break the reconnection attempt for anonymous access?

dbambus commented 4 months ago

Hey,

I ve rewritten the part in the latest unmerged pull request. But it is not yet tested ;-) maybe you can do that ?

Cheers David

dbambus commented 4 months ago

Fixed with https://github.com/ESPWortuhr/Multilayout-ESP-Wordclock/pull/397