Aircoookie / WLED

Control WS2812B and many more types of digital RGB LEDs with an ESP8266 or ESP32 over WiFi!
https://kno.wled.ge
MIT License
14.49k stars 3.1k forks source link

mqtt discovery #198

Closed mszymura closed 5 years ago

mszymura commented 5 years ago

hi, i have set the mqtt broker, but there is no input for the user and password so see in home assistant , using the mosquitto broker that the connection is being refused mqtt discovery enabled but this isnt showing up, getting this in the logs 1564870939: New connection from 192.168.0.134 on port 1883. 1564870939: Socket error on client , disconnecting.

using 0.84 bin file on d1mini

any help appreciated, thanks the lighting effects are amazing

Aircoookie commented 5 years ago

Hi, welcome to WLED! Username and password for MQTT are currently unsupported because of security concerns. However, since there is a large demand for this feature, I will add it in the next few weeks.

timothybrown commented 5 years ago

Hi, welcome to WLED! Username and password for MQTT are currently unsupported because of security concerns. However, since there is a large demand for this feature, I will add it in the next few weeks.

What sort of security concerns? To my mind, forcing me to enable anonymous mode on my broker (thus allowing any client to connect without a valid username/password) is a huge security concern.

Sure, you’d be storing the password in plaintext on the ESP device, but that’s why you setup a unique user for each client on your broker, and limit that user to specific topics. The MQTT-Async library already supports authentication, so I see no real reason not to implement it.

I recently added MQTT support to another ESP project called IRBlaster (it’s basically an IR TX/RX gateway), so I’d be happy to do the same for WLED if you’d like. :)

I had already planned to revamp the Home Assistant support in WLED, which requires digging into the MQTT portion anyway, so while I’m there it wouldn’t be much trouble. (I want to add proper state support, as right now the light status isn’t correctly reported to HA, plus the command and state topic hierarchies need to be re-ordered a bit. I’ll open a new issue on that with my proposed changes.)

brentbrooks70 commented 5 years ago

I think, if I remember previous posts correctly, that he was concerned that credentials being passed are in clear and plain text. Anyone snooping would be able to retrieve your credentials and either play man in the middle or completely hijack your device. This is of course a bigger issue if your broker and end device are not in the same location or network and is routed via a public network. I have been anxiously awaiting full login support via WLED for a while as the broker I use (QNAP’s QIOT Suite) requires ClientID, ClientName, and password that it generates when you add a new device, so I haven’t been able to integrate this amazing platform fully into my environment yet....Hope that helps.

timothybrown commented 5 years ago

Yeah, that does help! (I figured it was the password being stored and transferred in plaintext that was the issue.)

Personally, I don't really think this is much of an issue, for a few reasons. Let me preface this by saying everyone has different ideas and opinions on security, this is just my take after 20 years in the industry:

1) Some authentication is always better than no authentication. This at least prevents easy access to your broker. (I have locks on my doors because it stops someone from simply walking up to my house, opening the door and walking out with my TV. Locks don't stop someone from kicking in the door. If an attacker is determined enough, they will get in given enough time.) 2) If your broker and device are on different private networks you should be using a VPN or SSH tunnel to bridge them, and not simply opening ports and routing traffic across the internet. 3) If you're running your own broker (Mosquitto, etc.) and it's only accessible on your LAN, transmitting the password in plaintext isn't much of an issue. (If someone has access to your private network and is performing a MitM attack to gain your MQTT password, then not supporting authentication doesn't really help, because it forces you to enable anonymous mode on the broker, which allows unrestricted access anyway.)

Ideally I'd have my MQTT broker setup to only allow authenticated, TLS connections from specific MAC addresses. Unfortunately, there's some overhead involved in TLS which makes it hard to implement on small devices like ESP microcontrollers. That said, using authentication and a MAC whitelist is better than nothing.

I went ahead and spent a couple of hours this afternoon getting authentication going on WLED. Right now it's working, but I want to shake it down for another 24 hours and make a few small changes.

@brentbrooks70 Do you compile from source or use the binaries? If it's the latter I can provide binaries if you want to help test the changes. I use the bleeding edge release of Mosquito, so I'd like to test it on other brokers before submitting this as an official pull request.

brentbrooks70 commented 5 years ago

I compile my own as I have some customizations on all but 1 WLED device. I would be willing to test for both ESP8266 & ESP32 if needed. The broker I use is Mosquito but I have no method of determining what version it is as it runs as a separate docker instance that is hidden from the OS on my NAS/SAN device and is integrated with Node-Red and Freeboard, it is secured with Its own certificates so I use it for all public access and have it communicate inside my network (only DHCP reservations with MAC ACLs on wireless) to “secure” the internal environment. Should I just pull a copy/clone your repository for testing? Sorry if my dev terminology is not the greatest but I am a switch/router guy myself and dev is just a hobby.

timothybrown commented 5 years ago

No worries, your terminology is fine!

I'll put a fork of WLED with my changes on my GitHub for you tonight. You can clone that and merge any of your changes in. There are only around 5 files that contain the authentication changes, so if you want you can just copy them over. (Or just customize and build my version, whatever works best for you!)

What NAS/SAN device are you using? Sounds like it’s pretty full featured!

Do you have access to the logs for Mosquitto? Being able to monitor the logs could be useful to make sure the ESP is staying connected to your broker. (When initially adding the authentication changes I ran into an issue where the client wasn’t staying reliably connected. It appears to be fixed now, but more verification is always helpful!)

I'll bump this when it's up.

timothybrown commented 5 years ago

Okay, I've pushed the changes to my repository: https://github.com/timothybrown/WLED.git

Make sure you checkout the mqttauth branch (git checkout mqttauth) before compiling! Once you flash it, bring up the WLED web UI and go to Settings -> Sync Interfaces and scroll to the MQTT section. You should now have fields for an MQTT Username, Password and Client ID. If the username or password fields are blanked out it will disable authentication; the client ID now defaults to "WLED-Last Six Digits of MAC" but can be changed if you want (blanking it out will restore the default). You need to reboot your device after entering and saving the credentials.

Also, just a warning: If you decide to go back to the official release (or when my pull request is merged), you may have to use the restore defaults before doing an OTA upgrade. I’m storing the MQTT credentials in EEPROM and just picked the next free block of space. @Aircoookie may want to map it somewhere else. (This only applies to an OTA upgrade; if you physically hook your ESP up via USB it should erase the flash before reprogramming anyway.)

Let me know what you think!

lubeda commented 5 years ago

I tried to test the mqtt setting but the broker field is to short to enter my broker string

i tried to use user:password@ip-address

timothybrown commented 5 years ago

I tried to test the mqtt setting but the broker field is to short to enter my broker string

i tried to use user:password@ip-address

From my repository? You should have separate fields for the user and password.

Make sure you actually checkout the mqttauth branch once you clone my repository, as specified in my last post.

33FC97DE-079C-4448-A200-8C5042636251

brentbrooks70 commented 5 years ago

I also cannot enter the entire name or password in web fields my username is 36 characters long and my password is 34 characters. I did change lines 204-209 in WLED00 to reflect the larger sizes (character count + 1) for each field as in

char mqttDeviceTopic[33] = "qiot/things/admin/Temps1/OutLog"; char mqttGroupTopic[33] = "wled/all"; char mqttServer[33] = "xxx.xxx.xxx.xxx"; char mqttUser[37] = "1b92b15c-3636-4ca6-a63d-bc943aa5b132"; char mqttPass[35] = "r:956cf3d3ca53e0790f64ffecf8998369"; char mqttClientID[33] = "Temps1_1565891685"; (remember these are auto-generated by the server so I have no control over their length or content, I have regenerated the credentials so these are no longer valid, or I wouldn't post them)

I also changed line 5 in WELD7_MQTT to

define WLED_MQTT_PORT 21883

as this is the port my server uses.

But when I look at the sync setup page I still see a WLED-last6ofMAC as Client ID and wled/entireMAC. as Device topic. Could this be due to the sizes not fitting on the web interface, each field can only hold 32 characters it looks like. If you would like any screenshots or debug logs let me know but as of now, I cannot get this to connect to my broker or even show the settings I hardcoded.

running on NodeMCU ESP8266 12-f ESP8266 Core 2.4.2 Arduino 1.8.9 Compiled using VisualMicro running on Visual Studio Enterprise 2019

Thanks for all the effort and assistance

timothybrown commented 5 years ago

@brentbrooks70 There's other places in the code you have to change the length. Basically, the code that copies the variables from memory to the web form is set to 33 characters (32 + null terminator) and the code to store the variables into EEPROM is set with a 32 character limit (we don't store the null terminator).

If I push all the MQTT fields up to 40 characters, do you think that would be sufficient? We're starting to bump up against the limits of the EEPROM (which is set at 2560 bytes, we're using 2498 bytes). With 40 characters for all six saved MQTT fields we'd be at 2546 bytes, plus another 6 bytes for the port.

Ideally, once SPIFFs support is fully complete we could expand the fields to be 256 or more characters.

I completely forgot about the port! I'll add a field for that in the web UI as well, thanks for reminding me!

brentbrooks70 commented 5 years ago

I currently have 11 devices provisioned from this server and all of them are 36 and 34 characters for user and pass respectively so 40 should be more than enough for my needs. Client ID is Thing name in the GUI + "_10digits" ie Temps1_156589168 as above so as long as I keep the names to less than 28 characters I should not have an issue with 40 characters there either. Let me know when you would like me to test again and thanks for the work!!!

timothybrown commented 5 years ago

@brentbrooks70 Okay, I just updated the code and bumped the username, password and client ID fields to 40 characters. The server, device topic and group topic remain at 32 characters. Port is restricted to 5 characters. (The math in my previous post was incorrect and I didn't have enough storage to move them all up to 40 characters. We still have a tiny bit left, so I can still bump one up if required. Let me know!)

Note, your web UI may be confused after flashing this update. You'll need to go in and re-enter the MQTT settings and save them, then reboot the device for the changes to be saved in EEPROM.

mqttwled

brentbrooks70 commented 5 years ago

It is checking in and sending data via MQTT!!! Now I just need to read up on what it's sending.... I was expecting JSON like what comes from IP/JSON/State but instead, I got: 50255000025511016004751282-1010WLED Light which looks closer to the XML response, I think. Heading to Wiki now to read up, will continue to test on this ESP8266 and will load on an ESP32 and report back tomorrow with any issues or even if not.

Thanks again for the assistance

timothybrown commented 5 years ago

Awesome! Glad it’s working for you now!

Yes, the wiki should help understand what’s it’s sending; you may also want to check the Home Assistant Discovery section of the wiki too (HA support uses MQTT). Finally, there’s some comments in the wled17_mqtt.ino file that may help as well.

Let me know how it goes with the other devices!

Aircoookie commented 5 years ago

First of all, please excuse my non-participation in the issue so far! @timothybrown thank you very much for your hard work on #202 ! Will add another commit later today (add security warning, change MQTT port to number field, and improving security a tiny bit (not sending plaintext MQTT password everytime the sync settings are opened)) and then merge it in!

@brentbrooks70

50255000025511016004751282-1010WLED Light

Yes, the /api MQTT topic contains the XML api response. What you got as a response is definitely a bug though (it is the XML, but without any XML keys like <a></a>). I could also reproduce this with Chrome (#204), but inspecting the raw packet suggests that the ESP is sending valid data. Please add info to that issue if the problem persists!

brentbrooks70 commented 5 years ago

@timothybrown I have put this method of connectivity through its paces today and I would say so far it has been rock solid (other than the #204 issue). Thanks again for your work on this and immediate help in fixing the field sizes.

timothybrown commented 5 years ago

@brentbrooks70 Thanks for letting me know and you're very welcome!

stagea09 commented 5 years ago

I have tested this also with homeassistant and it's all working perfectly :)

timothybrown commented 5 years ago

@stagea09 Awesome, thanks for the confirmation!

My changes (#202) have now been merged into master, so this issue should be resolved. :)