xoseperez / espurna

Home automation firmware for ESP8266-based devices
http://tinkerman.cat
GNU General Public License v3.0
3k stars 638 forks source link

Current status of MQTT SSL #1465

Open ghost opened 5 years ago

ghost commented 5 years ago

Running latest master branch as of today, I'm not sure anymore if the SSL support still conflicts with the web UI regardless of the platform used (I'm building with 180).

It would be nice to have a current document on SSL on its own page, as well as tweaks to enable HTTP access for the UI but MQTT over SSL, to minimize the performance requirements.

mcspr commented 5 years ago

Some notes after testing:

All tests done with local server, because I could not get cloudmqtt.com to work. It just crashes the board (or fails to connect at the best of times). Tried async/axtls builds, new sync/bearssl, even tried different firmware from mongoose os - nothing works with them :/

Pubsubclient + bearssl combo can also use CA cert instead of fingerprinting, either as hardcoded string or spiffs file. Also, fingerprinting api is different and there is no need to manually do it (client.connect, check fp, mqtt.setClient, mqtt.connect repeat)

stale[bot] commented 5 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

stale[bot] commented 5 years ago

This issue will be auto-closed because there hasn't been any activity for two months. Feel free to open a new one if you still experience this problem.

Niek commented 5 years ago

My experience with core 2.4.2:

I tried testing with core 2.5.0 as well, but unfortunately the binary grows too large to do OTA on a 1MB board - no matter which MQTT library I use.

mcspr commented 5 years ago

pretty sure this is due to conflicts with ESPAsyncTCP

Like, what conflicts? I've seen the comment, but I still do not quite understand what conflict is there.

In general with BearSSL:

Niek commented 5 years ago

MFLN is interesting - if you compile your broker against OpenSSL 1.1 it should work out of the box. It should be quite easy to do a couple of quick probeMaxFragmentLength() calls in the MQTT module before connecting.

As for conflicts: whenever I try to use PubSubClient (MQTT_USE_ASYNC=0) I cannot connect to any MQTT broker - the _mqtt_client_secure.connect() call always fails. Since it's suggested in the header files, I assumed the WifiClientSecure conflicts with WifiClient used in ESPAsyncTCP.

Niek commented 5 years ago

Just an update after lots of experimentation: PubSubClient SSL (MQTT_USE_ASYNC=0) does not work in any of my cases. When I compile with -DDEBUG_ESP_SSL -DDEBUG_ESP_TLS_MEM I see the following errors:

[MQTT] Connecting as user X
please start sntp first !
State:  sending Client Hello (1)
Error: connection lost

When using AsyncMqttClient, SSL works but only a few packets. It disconnects and then reconnects in a loop every x packets. I tested with Arduino core 2.4.2 and 2.5.0 (axTLS only, BearSSL is too heavy) and LWIP 1.4 and 2.0 - no difference.

mcspr commented 5 years ago

iit i've already mentioned the problem - you need to manually supply NTP time to the WiFiClient instance. "please start sntp first !" error is coming from us not using LWIP implementation of NTP client, it expects us to call configTime() somewhere (example).

With NtpClientLib flow should be

  1. MQTT module waits for ntpSynced() before starting connection routine
  2. mqtt_client.setX509Time(now()). setInsecure() / set trust anchors
  3. try connecting

I'll try to update pubsubclient support for this while keeping fingerprinting / support old Core versions.

Niek commented 5 years ago

Hmm, but this all with axTLS - which doesn't even have setX509Time(). Also manually calling configTime() doesn't seem to resolve anything - but maybe I'm overseeing something.

mcspr commented 5 years ago

Ah. Time error would only come with bearssl, when it calls time() internally searching time(... in https://github.com/earlephilhower/bearssl-esp8266 which comes from this override https://github.com/esp8266/Arduino/blob/3b9db65ea33890e9fa5e911ea4170e4b31fd03be/cores/esp8266/time.cpp#L83 and the origin of the error https://github.com/esp8266/Arduino/blob/3b9db65ea33890e9fa5e911ea4170e4b31fd03be/tools/sdk/lwip/src/core/sntp.c#L625 (that is lwip1.4, but lwip2 provides the same function)

What server config / could mqtt provider do you use for testing? I don't remember any reconnects with local mosquitto + async mqtt

Niek commented 5 years ago

I see slightly different output in LWIP2 vs LWIP 1.4:

Core 2.4.2 / 2.5.0, LWIP 2 with axTLS:

:close
:ur 1
:del
:ref 1
:ref 2
State:  sending Client Hello (1)
Error: connection lost
:ur 2

Core 2.4.2 / 2.5.0, LWIP 1.4 with axTLS:

:close
:ur 1
:del
:ref 1
:ref 2
please start sntp first !
State:  sending Client Hello (1)
Error: connection lost
:ur 2

This is just calling connect() on the axTLS::WiFiClientSecure object. It doesn't matter which host I use, I tried a lot of them and all connect() calls fail. If I compile a sample like https://github.com/esp8266/Arduino/blob/master/libraries/ESP8266WiFi/examples/HTTPSRequestAxTLS/HTTPSRequestAxTLS.ino it works fine, so I think some library used in Espurna is interfering with WifiClientSecure like is suggested in the config.

Edit: Finally got it working. I can confirm that it's really ESPAsyncTCP that is messing with WifiClientSecure. The following config works for me:

#define TELNET_SUPPORT              0
#define WEB_SUPPORT                 0
#define TERMINAL_SUPPORT            0
#define OTA_MQTT_SUPPORT            0
#define DEBUG_TELNET_SUPPORT        0
#define ALEXA_SUPPORT               0
#define INFLUXDB_SUPPORT            0
#define THINGSPEAK_SUPPORT          0
mcspr commented 5 years ago

🤷‍♂️ Opposite results. BearSSL works, axTLS variant does not. I would not put async as a whole as the reason. We are using more RAM than basic example, and do a lot more inside loop() too. If you are not connected via async library, web or telnet, how would it conflict?

Using 2.5.0. axTLS enabled via -DUSING_AXTLS, manually including WiFiClientSecureAxTLS.h, using namespace axTLS;

BearSSL test that worked:

There's also a small bug with sync client - _mqtt_last_connection = millis(); needs to be set if we can't connect. It messes up timing really bad, forcing reconnection every loop after some time, if server is not responding.

Niek commented 5 years ago

Strange. What makes it complicated is that it depends on a lot of factors, like if the broker supports one of the few ciphers that axTLS supports (e.g. AES128-SHA or AES265-SHA). I have a WIP branch @ https://github.com/Niek/espurna/tree/mqtt-fixes with some fixes. I added the https://github.com/256dpi/arduino-mqtt library as well, since it's much better maintained than AsyncMqttClient and PubSubClient. Next step would be to add full support for BearSSL and a configurable setting for validation (non/insecure, fingerprint, chain, etc).

mcspr commented 5 years ago

Huh.

Tests done on Fedora 29 mosquitto-1.5.8-1.fc29.x86_64 openssl-1.1.1b-3.fc29.x86_64

Looking at debug log, connection fails on Hello step. Packet capture shows that server answers:

Cipher Suite: TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (0xc030)

Which I don't see here: https://github.com/igrr/axtls-8266/blob/master/README.md

But!, It is in the cipher list inside client's Hello: ``` Cipher Suites (45 suites) Cipher Suite: TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256 (0xcca9) Cipher Suite: TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256 (0xcca8) Cipher Suite: TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256 (0xc02b) Cipher Suite: TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (0xc02f) Cipher Suite: TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384 (0xc02c) Cipher Suite: TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (0xc030) <-------- Cipher Suite: TLS_ECDHE_ECDSA_WITH_AES_128_CCM (0xc0ac) Cipher Suite: TLS_ECDHE_ECDSA_WITH_AES_256_CCM (0xc0ad) Cipher Suite: TLS_ECDHE_ECDSA_WITH_AES_128_CCM_8 (0xc0ae) Cipher Suite: TLS_ECDHE_ECDSA_WITH_AES_256_CCM_8 (0xc0af) Cipher Suite: TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256 (0xc023) Cipher Suite: TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256 (0xc027) Cipher Suite: TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA384 (0xc024) Cipher Suite: TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384 (0xc028) Cipher Suite: TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA (0xc009) Cipher Suite: TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA (0xc013) Cipher Suite: TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA (0xc00a) Cipher Suite: TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA (0xc014) Cipher Suite: TLS_ECDH_ECDSA_WITH_AES_128_GCM_SHA256 (0xc02d) Cipher Suite: TLS_ECDH_RSA_WITH_AES_128_GCM_SHA256 (0xc031) Cipher Suite: TLS_ECDH_ECDSA_WITH_AES_256_GCM_SHA384 (0xc02e) Cipher Suite: TLS_ECDH_RSA_WITH_AES_256_GCM_SHA384 (0xc032) Cipher Suite: TLS_ECDH_ECDSA_WITH_AES_128_CBC_SHA256 (0xc025) Cipher Suite: TLS_ECDH_RSA_WITH_AES_128_CBC_SHA256 (0xc029) Cipher Suite: TLS_ECDH_ECDSA_WITH_AES_256_CBC_SHA384 (0xc026) Cipher Suite: TLS_ECDH_RSA_WITH_AES_256_CBC_SHA384 (0xc02a) Cipher Suite: TLS_ECDH_ECDSA_WITH_AES_128_CBC_SHA (0xc004) Cipher Suite: TLS_ECDH_RSA_WITH_AES_128_CBC_SHA (0xc00e) Cipher Suite: TLS_ECDH_ECDSA_WITH_AES_256_CBC_SHA (0xc005) Cipher Suite: TLS_ECDH_RSA_WITH_AES_256_CBC_SHA (0xc00f) Cipher Suite: TLS_RSA_WITH_AES_128_GCM_SHA256 (0x009c) Cipher Suite: TLS_RSA_WITH_AES_256_GCM_SHA384 (0x009d) Cipher Suite: TLS_RSA_WITH_AES_128_CCM (0xc09c) Cipher Suite: TLS_RSA_WITH_AES_256_CCM (0xc09d) Cipher Suite: TLS_RSA_WITH_AES_128_CCM_8 (0xc0a0) Cipher Suite: TLS_RSA_WITH_AES_256_CCM_8 (0xc0a1) Cipher Suite: TLS_RSA_WITH_AES_128_CBC_SHA256 (0x003c) Cipher Suite: TLS_RSA_WITH_AES_256_CBC_SHA256 (0x003d) Cipher Suite: TLS_RSA_WITH_AES_128_CBC_SHA (0x002f) Cipher Suite: TLS_RSA_WITH_AES_256_CBC_SHA (0x0035) Cipher Suite: TLS_ECDHE_ECDSA_WITH_3DES_EDE_CBC_SHA (0xc008) Cipher Suite: TLS_ECDHE_RSA_WITH_3DES_EDE_CBC_SHA (0xc012) Cipher Suite: TLS_ECDH_ECDSA_WITH_3DES_EDE_CBC_SHA (0xc003) Cipher Suite: TLS_ECDH_RSA_WITH_3DES_EDE_CBC_SHA (0xc00d) Cipher Suite: TLS_RSA_WITH_3DES_EDE_CBC_SHA (0x000a) ```

Most basic mosquitto config. Changing ciphers does nothing, client disconnects right after server Hello is done.

port 8883
cafile ca.crt
certfile server.crt
keyfile server.key
#this does not help either:
#ciphers AES256-SHA256:AES-SHA:AES128-SHA256:AES128-SHA

edit: Which is probably something broken with 2.5.0 + axtls client, because older Cores work fine.

Niek commented 5 years ago

I just tested both core 2.4.2 (axTLS) and 2.5.0 (axTLS and BearSSL), and both work fine for me (BearSSL needs the setInsecure() call though), axTLS on 2.5.0 works only with this explicit declaration:

#define USING_AXTLS // do not use BearSSL
#include "WiFiClientSecureAxTLS.h"
axTLS::WiFiClientSecure _mqtt_client_secure;

I pushed the changes to https://github.com/Niek/espurna/tree/mqtt-fixes

mcspr commented 5 years ago

Ok. BTW, I had changed MQTT settings bit in #1701. I'd like to merge that to fix reloading, if you can review for any issues it would be appreciated. If the mqtt-fixes tree can work with all Cores, it would be great

Brief look ArduinoMQTT, I think it still applies. It copies some of params, and we can't know which were set. e.g. hostname and will topic are private members with no public getters. Otherwise, I am a big fan of WiFiClient interface. I wish both async clients were a bit more pluggable, so it would not be a must to change async-mqtt-client code :(

sidenote: In #1693 I tried a different approach at splitting implementations. I am a bit scared of too much nested ifdefs, but it might be a personal preference

ghost commented 5 years ago

If this gains traction, especially @Niek 's changes, it could be a positive development to merge. Because the telnet interface is incomplete, and insecure, not being able to operate the web interface over SSL is an issue, especially since MQTT with SSL is probably a definite must at this point. Relying on the security of the physical transport link (wireless) to safeguard MQTT credentials is a dumb idea.

I haven't had time to test any of these yet, but I'm keeping a watchful eye on this thread and hope @xoseperez does too.

mcspr commented 5 years ago

Regarding secure WiFiClient in general, there are two annoying problems with old Cores: https://github.com/esp8266/Arduino/issues/5786

Verifier struct(s) not freed before client is destroyed (only needed when connecting, not after) Client might hang under certain conditions (see second PR)

Niek commented 5 years ago

Regarding secure WiFiClient in general, there are two annoying problems with old Cores: esp8266/Arduino#5786

Verifier struct(s) not freed before client is destroyed (only needed when connecting, not after) Client might hang under certain conditions (see second PR)

That seems to be fixed in core 2.5.2 / PIO 2.2.0 - I'm adding a PR now to add support for the last 2 releases. I will also soon post a PR with the MQTT changes.

Niek commented 5 years ago

Linking this PR in here: https://github.com/xoseperez/espurna/pull/1829

mcspr commented 5 years ago

Followup on the https://github.com/xoseperez/espurna/pull/1829#issuecomment-523919225, not to mix PR comments and general info.

Tasmota implementation includes bearssl & wificlientsecure into the tree (which reminds me of the ESPAsyncTCP fork...), and ignores X509List class so that cert chain is read directly from the progmem pointer: https://github.com/arendst/Sonoff-Tasmota/blob/5cb863d35b8f7cc67497d937ab50c299b301e290/sonoff/xdrv_02_mqtt.ino#L217 https://github.com/arendst/Sonoff-Tasmota/blob/46d2cb8d0b58a68ac91a531f355730bca5a75309/sonoff/WiFiClientSecureLightBearSSL.cpp#L237 https://github.com/arendst/Sonoff-Tasmota/blob/46d2cb8d0b58a68ac91a531f355730bca5a75309/sonoff/WiFiClientSecureLightBearSSL.cpp#L836

original method accepts helper class X509List, which is a really simple wrapper for the C x509 struct https://github.com/esp8266/Arduino/blob/8f45a0fb91621de2c8bedb5723f2eb3b527706c4/libraries/ESP8266WiFi/src/WiFiClientSecureBearSSL.h#L94 https://github.com/esp8266/Arduino/blob/55539ae941bcf5a84ba6c53f1ea8bce145882424/libraries/ESP8266WiFi/src/WiFiClientSecureBearSSL.cpp#L1019 https://github.com/esp8266/Arduino/blob/55539ae941bcf5a84ba6c53f1ea8bce145882424/libraries/ESP8266WiFi/src/BearSSLHelpers.h#L113 And current bearssl in the Core should be working ok? https://github.com/earlephilhower/bearssl-esp8266/pull/15 I am sort of assuming that the version they are using is from the core, can't find any pre-built binaries in the tree there. Nontheless, "t_bearssl.h" is used instead of "bearssl.h" for bearssl C library APIs

This approach is only useful if cert is stored inside of flash sector, skipping costly RAM copy. Because the other option is to use FS (SPIFFS / LittleFS) and a single certs.ar container, where CertStore helper comes into play. While full chain is stored in the file, we still seem to have to load the required CA cert into the RAM for validation (i think it is like that? https://github.com/esp8266/Arduino/blob/55539ae941bcf5a84ba6c53f1ea8bce145882424/libraries/ESP8266WiFi/src/CertStoreBearSSL.cpp#L182)

Sources: https://github.com/esp8266/Arduino/blob/master/libraries/ESP8266WiFi/src/WiFiClientSecureBearSSL.cpp https://github.com/esp8266/Arduino/blob/master/libraries/ESP8266WiFi/src/CertStoreBearSSL.cpp https://github.com/arendst/Sonoff-Tasmota/blob/development/sonoff/WiFiClientSecureLightBearSSL.cpp https://github.com/arendst/Sonoff-Tasmota/tree/development/lib/bearssl-esp8266

Niek commented 5 years ago

The Tasmota way of PEM->DER->base64->config sounds overly complicated to me, in my opinion it would make much more sense to upload the certificate files through the web UI and store it somewhere on the FS (sample code: https://github.com/esp8266/Arduino/blob/0937b076c8ac31d3dbfe7ed4ccc3a2efd7378396/libraries/ESP8266WiFi/examples/BearSSL_CertStore/BearSSL_CertStore.ino#L141-L150). Or maybe even offer the option to auto-fetch the server certificate once and store it for future connects.