Secure client / server cleanup (#2016)

* web: revert changed from secure_client patch

* ota/httpupdate: use SecureClient

* ota/httpupdate: use scheduled function on mqtt event

* config: finish up 590282e changes + add warnings

* typo fix

* use bind instead of c++14 magic, use debug text from secureclient

* bump

* actually use the locking

README.md

@@ -268,7 +268,7 @@ Here is the list of supported hardware. For more information please refer to the
 |![Itead Sonoff 4CH](images/devices/itead-sonoff-4ch.jpg)|![Itead Sonoff 4CH Pro](images/devices/itead-sonoff-4ch-pro.jpg)||
 |**Itead Sonoff 4CH**|**Itead Sonoff 4CH Pro**||
 |![Allterco Shelly 1 / 1PM](images/devices/allterco-shelly1.jpg)|![Allterco Shelly 2 / 2.5](images/devices/allterco-shelly2.jpg)|![Jan Goedeke Wifi Relay (NO/NC)](images/devices/jangoe-wifi-relay.jpg)|
-|**Alterco Shelly1**|**Alterco Shelly2**|**Jan Goedeke Wifi Relay (NO/NC)**|
+|**Alterco Shelly 1 / 1PM**|**Alterco Shelly 2 / 2.5**|**Jan Goedeke Wifi Relay (NO/NC)**|
 |![EXS Wifi Relay v3.1](images/devices/exs-wifi-relay-v31.jpg)|![EXS Wifi Relay v5.0](images/devices/exs-wifi-relay-v50.jpg)|![Jorge García Wifi + Relays Board Kit](images/devices/jorgegarcia-wifi-relays.jpg)|
 |**EXS Wifi Relay v3.1**|**EXS Wifi Relay v5.0**|**Jorge García Wifi + Relays Board Kit**|
 |![Allnet ESP8266-UP-Relay](images/devices/allnet-esp8266-up-relay.jpg)|![Bruno Horta's OnOfre](images/devices/bh-onofre.jpg)|![Luani HVIO](images/devices/luani-hvio.jpg)|

code/espurna/config/dependencies.h

@@ -5,11 +5,6 @@
 // Configuration settings are in the general.h file
 //------------------------------------------------------------------------------
 
-#if defined(ASYNC_TCP_SSL_ENABLED) && SECURE_CLIENT == SECURE_CLIENT_NONE
-#undef SECURE_CLIENT
-#define SECURE_CLIENT               SECURE_CLIENT_AXTLS
-#endif
-
 #if DEBUG_TELNET_SUPPORT
 #undef TELNET_SUPPORT
 #define TELNET_SUPPORT              1
@@ -70,13 +65,6 @@
 #define MQTT_SUPPORT                1               // If Home Assistant enabled enable MQTT
 #endif
 
-#if SECURE_CLIENT != SECURE_CLIENT_AXTLS
-#if THINGSPEAK_USE_SSL && THINGSPEAK_USE_ASYNC
-#undef THINGSPEAK_SUPPORT                       
-#define THINGSPEAK_SUPPORT          0               // Thingspeak in ASYNC mode requires SECURE_CLIENT_AXTLS
-#endif
-#endif
-
 #if THINGSPEAK_SUPPORT
 #undef BROKER_SUPPORT
 #define BROKER_SUPPORT              1               // If Thingspeak enabled enable BROKER
@@ -124,3 +112,20 @@
 #define BROKER_SUPPORT              1           // Broker is required to process relay & lights events
 #endif
 
+// TODO: clean-up SSL_ENABLED and USE_SSL settings for 1.15.0
+#if ASYNC_TCP_SSL_ENABLED && SECURE_CLIENT == SECURE_CLIENT_NONE
+#undef SECURE_CLIENT
+#define SECURE_CLIENT               SECURE_CLIENT_AXTLS
+#endif
+
+#if THINGSPEAK_USE_SSL && THINGSPEAK_USE_ASYNC && (!ASYNC_TCP_SSL_ENABLED)
+#warning "Thingspeak in ASYNC mode requires a globally defined ASYNC_TCP_SSL_ENABLED=1"
+#undef THINGSPEAK_SUPPORT
+#define THINGSPEAK_SUPPORT          0               // Thingspeak in ASYNC mode requires ASYNC_TCP_SSL_ENABLED
+#endif
+
+#if WEB_SUPPORT && WEB_SSL_ENABLED && (!ASYNC_TCP_SSL_ENABLED)
+#warning "WEB_SUPPORT with SSL requires a globally defined ASYNC_TCP_SSL_ENABLED=1"
+#undef WEB_SSL_ENABLED
+#define WEB_SSL_ENABLED          0               // WEB_SUPPORT mode th SSL requires ASYNC_TCP_SSL_ENABLED
+#endif

code/espurna/config/deprecated.h

@@ -42,3 +42,8 @@
 #ifdef HOMEASSISTANT_PAYLOAD_NOT_AVAILABLE
 #warning HOMEASSISTANT_PAYLOAD_NOT_AVAILABLE is deprecated! Global MQTT_STATUS_OFFLINE is used instead
 #endif
+
+// 1.14.0 adds SecureClient
+#if MQTT_SUPPORT && MQTT_LIBRARY == MQTT_LIBRARY_ASYNCMQTT_CLIENT && ASYNC_TCP_SSL_ENABLED
+#warning "Current implementation of AsyncMqttClient with axTLS is no longer supported. Consider switching to the SECURE_CLIENT configuration with MQTT_LIBRARY_ARDUINOMQTT or MQTT_LIBRARY_PUBSUBCLIENT. See: https://github.com/xoseperez/espurna/issues/1465"
+#endif

code/espurna/ota_httpupdate.ino

@@ -16,9 +16,11 @@ Copyright (C) 2019 by Maxim Prokhorov <prokhorov dot max at outlook dot com>
 
 #include <ESP8266HTTPClient.h>
 #include <ESP8266httpUpdate.h>
+#include <Schedule.h>
 
 #include "libs/URL.h"
 #include "libs/TypeChecks.h"
+#include "libs/SecureClientHelpers.h"
 
 #if SECURE_CLIENT != SECURE_CLIENT_NONE
 
@@ -31,6 +33,10 @@ Copyright (C) 2019 by Maxim Prokhorov <prokhorov dot max at outlook dot com>
 
 #endif // SECURE_CLIENT != SECURE_CLIENT_NONE
 
+// -----------------------------------------------------------------------------
+// Configuration templates
+// -----------------------------------------------------------------------------
+
 template <typename T>
 void _otaFollowRedirects(const std::true_type&, T& instance) {
     instance.followRedirects(true);
@@ -64,6 +70,49 @@ namespace ota {
     using has_WiFiClient_argument = is_detected<has_WiFiClient_argument_t, T>;
 }
 
+// -----------------------------------------------------------------------------
+// Settings helper
+// -----------------------------------------------------------------------------
+
+#if SECURE_CLIENT == SECURE_CLIENT_AXTLS
+SecureClientConfig _ota_sc_config {
+    "OTA",
+    []() -> String {
+        return String(); // NOTE: unused
+    },
+    []() -> int {
+        return getSetting("otaScCheck", OTA_SECURE_CLIENT_CHECK).toInt();
+    },
+    []() -> String {
+        return getSetting("otaFP", OTA_FINGERPRINT);
+    },
+    true
+};
+#endif
+
+#if SECURE_CLIENT == SECURE_CLIENT_BEARSSL
+SecureClientConfig _ota_sc_config {
+    "OTA",
+    []() -> int {
+        return getSetting("otaScCheck", OTA_SECURE_CLIENT_CHECK).toInt();
+    },
+    []() -> PGM_P {
+        return _ota_client_trusted_root_ca;
+    },
+    []() -> String {
+        return getSetting("otaFP", OTA_FINGERPRINT);
+    },
+    []() -> uint16_t {
+        return getSetting("otaScMFLN", OTA_SECURE_CLIENT_MFLN).toInt();
+    },
+    true
+};
+#endif
+
+// -----------------------------------------------------------------------------
+// Generic update methods
+// -----------------------------------------------------------------------------
+
 void _otaClientRunUpdater(WiFiClient* client, const String& url, const String& fp = "") {
 
     UNUSED(client);
@@ -76,8 +125,6 @@ void _otaClientRunUpdater(WiFiClient* client, const String& url, const String& f
 
     // TODO: support currentVersion (string arg after 'url')
     // NOTE: ESPhttpUpdate.update(..., fp) will **always** fail with empty fingerprint
-    // NOTE: It is possible to support BearSSL with 2.4.2 by using uint8_t[20] instead of String for fingerprint argument
-
     _otaFollowRedirects(ota::has_followRedirects<decltype(ESPhttpUpdate)>{}, ESPhttpUpdate);
 
     ESPhttpUpdate.rebootOnUpdate(false);
@@ -91,6 +138,8 @@ void _otaClientRunUpdater(WiFiClient* client, const String& url, const String& f
             result = ESPhttpUpdate.update(url);
         }
     #else
+    // TODO: implement through callbacks?
+    //       see https://github.com/esp8266/Arduino/pull/6796
         result = _otaClientUpdate(ota::has_WiFiClient_argument<decltype(ESPhttpUpdate)>{}, ESPhttpUpdate, client, url);
     #endif
 
@@ -123,65 +172,21 @@ void _otaClientFromHttp(const String& url) {
 
 void _otaClientFromHttps(const String& url) {
 
-    int check = getSetting("otaScCheck", OTA_SECURE_CLIENT_CHECK).toInt();
-    bool settime = (check == SECURE_CLIENT_CHECK_CA);
-
-    if (!ntpSynced() && settime) {
-        DEBUG_MSG_P(PSTR("[OTA] Time not synced!\n"));
+    // Check for NTP early to avoid constructing SecureClient prematurely
+    const int check = _ota_sc_config.on_check();
+    if (!ntpSynced() && (check == SECURE_CLIENT_CHECK_CA)) {
+        DEBUG_MSG_P(PSTR("[OTA] Time not synced! Cannot use CA validation\n"));
         return;
     }
 
     // unique_ptr self-destructs after exiting function scope
-    // create WiFiClient on heap to use less stack space
-    auto client = std::make_unique<BearSSL::WiFiClientSecure>();
-
-    if (check == SECURE_CLIENT_CHECK_NONE) {
-        DEBUG_MSG_P(PSTR("[OTA] !!! Connection will not be validated !!!\n"));
-        client->setInsecure();
-    }
-
-    if (check == SECURE_CLIENT_CHECK_FINGERPRINT) {
-        String fp_string = getSetting("otaFP", OTA_FINGERPRINT);
-        if (!fp_string.length()) {
-            DEBUG_MSG_P(PSTR("[OTA] Requested fingerprint auth, but 'otaFP' is not set\n"));
-            return;
-        }
-
-        uint8_t fp_bytes[20] = {0};
-        sslFingerPrintArray(fp_string.c_str(), fp_bytes);
-
-        client->setFingerprint(fp_bytes);
-    }
-
-    BearSSL::X509List *ca = nullptr;
-    if (check == SECURE_CLIENT_CHECK_CA) {
-        ca = new BearSSL::X509List(_ota_client_trusted_root_ca);
-        // because we do not support libc methods of getting time, force client to use ntpclientlib's current time
-        // XXX: local2utc method use is detrimental when DST happening. now() should be utc
-        client->setX509Time(ntpLocal2UTC(now()));
-        client->setTrustAnchors(ca);
-    }
-
-    // TODO: RX and TX buffer sizes must be equal?
-    const uint16_t requested_mfln = getSetting("otaScMFLN", OTA_SECURE_CLIENT_MFLN).toInt();
-    switch (requested_mfln) {
-        // default, do nothing
-        case 0:
-            break;
-        // match valid sizes only
-        case 512:
-        case 1024:
-        case 2048:
-        case 4096:
-        {
-            client->setBufferSizes(requested_mfln, requested_mfln);
-            break;
-        }
-        default:
-            DEBUG_MSG_P(PSTR("[OTA] Warning: MFLN buffer size must be one of 512, 1024, 2048 or 4096\n"));
+    // create the client on heap to use less stack space
+    auto client = std::make_unique<SecureClient>(_ota_sc_config);
+    if (!client->beforeConnected()) {
+        return;
     }
 
-    _otaClientRunUpdater(client.get(), url);
+    _otaClientRunUpdater(&client->get(), url);
 
 }
 
@@ -192,11 +197,12 @@ void _otaClientFromHttps(const String& url) {
 
 void _otaClientFromHttps(const String& url) {
 
-    const int check = getSetting("otaScCheck", OTA_SECURE_CLIENT_CHECK).toInt();
+    // Note: this being the legacy option, only supporting legacy methods on ESPHttpUpdate itself
+    //       no way to know when it is connected, so no afterConnected
+    const int check = _ota_sc_config.on_check();
+    const String fp_string = _ota_sc_config.on_fingerprint();
 
-    String fp_string;
     if (check == SECURE_CLIENT_CHECK_FINGERPRINT) {
-        fp_string = getSetting("otaFP", OTA_FINGERPRINT);
         if (!fp_string.length() || !sslCheckFingerPrint(fp_string.c_str())) {
             DEBUG_MSG_P(PSTR("[OTA] Wrong fingerprint\n"));
             return;
@@ -247,15 +253,6 @@ void _otaClientInitCommands() {
 #if (MQTT_SUPPORT && OTA_MQTT_SUPPORT)
 
 bool _ota_do_update = false;
-String _ota_url;
-
-void _otaClientLoop() {
-    if (_ota_do_update) {
-        _otaClientFrom(_ota_url);
-        _ota_do_update = false;
-        _ota_url = "";
-    }
-}
 
 void _otaClientMqttCallback(unsigned int type, const char * topic, const char * payload) {
 
@@ -264,11 +261,18 @@ void _otaClientMqttCallback(unsigned int type, const char * topic, const char *
     }
 
     if (type == MQTT_MESSAGE_EVENT) {
-        String t = mqttMagnitude((char *) topic);
-        if (t.equals(MQTT_TOPIC_OTA)) {
+        const String t = mqttMagnitude((char *) topic);
+        if (!_ota_do_update && t.equals(MQTT_TOPIC_OTA)) {
             DEBUG_MSG_P(PSTR("[OTA] Queuing from URL: %s\n"), payload);
+            // TODO: c++14 support is required for `[_payload = String(payload)]() { ... }`
+            //       c++11 also supports basic `std::bind(func, arg)`, but we need to reset the lock
             _ota_do_update = true;
-            _ota_url = payload;
+
+            const String _payload(payload);
+            schedule_function([_payload]() {
+                _otaClientFrom(_payload);
+                _ota_do_update = false;
+            });
         }
     }
 
@@ -289,7 +293,6 @@ void otaClientSetup() {
 
     #if (MQTT_SUPPORT && OTA_MQTT_SUPPORT)
         mqttRegister(_otaClientMqttCallback);
-        espurnaRegisterLoop(_otaClientLoop);
     #endif
 
 }

code/espurna/thinkspeak.ino

@@ -227,7 +227,7 @@ void _tspkPost() {
 
     _tspk_client_ts = millis();
 
-    #if SECURE_CLIENT == SECURE_CLIENT_AXTLS
+    #if THINGSPEAK_USE_SSL
         bool connected = _tspk_client->connect(THINGSPEAK_HOST, THINGSPEAK_PORT, THINGSPEAK_USE_SSL);
     #else
         bool connected = _tspk_client->connect(THINGSPEAK_HOST, THINGSPEAK_PORT);

code/espurna/web.ino

@@ -39,10 +39,10 @@ Copyright (C) 2016-2019 by Xose Pérez <xose dot perez at gmail dot com>
 
 #endif // WEB_EMBEDDED
 
-#if SECURE_CLIENT == SECURE_CLIENT_AXTLS & WEB_SSL_ENABLED
+#if WEB_SSL_ENABLED
 #include "static/server.cer.h"
 #include "static/server.key.h"
-#endif // SECURE_CLIENT == SECURE_CLIENT_AXTLS & WEB_SSL_ENABLED
+#endif // WEB_SSL_ENABLED
 
 // -----------------------------------------------------------------------------
 
@@ -194,7 +194,7 @@ void _onHome(AsyncWebServerRequest *request) {
 
     } else {
 
-        #if SECURE_CLIENT == SECURE_CLIENT_AXTLS
+        #if WEB_SSL_ENABLED
 
             // Chunked response, we calculate the chunks based on free heap (in multiples of 32)
             // This is necessary when a TLS connection is open since it sucks too much memory
@@ -235,7 +235,7 @@ void _onHome(AsyncWebServerRequest *request) {
 }
 #endif
 
-#if SECURE_CLIENT == SECURE_CLIENT_AXTLS & WEB_SSL_ENABLED
+#if WEB_SSL_ENABLED
 
 int _onCertificate(void * arg, const char *filename, uint8_t **buf) {
 
@@ -446,7 +446,7 @@ void webRequestRegister(web_request_callback_f callback) {
 }
 
 unsigned int webPort() {
-    #if SECURE_CLIENT == SECURE_CLIENT_AXTLS & WEB_SSL_ENABLED
+    #if WEB_SSL_ENABLED
         return 443;
     #else
         return getSetting("webPort", WEB_PORT).toInt();
@@ -497,7 +497,7 @@ void webSetup() {
     _server->onNotFound(_onRequest);
 
     // Run server
-    #if SECURE_CLIENT == SECURE_CLIENT_AXTLS & WEB_SSL_ENABLED
+    #if WEB_SSL_ENABLED
         _server->onSslFileRequest(_onCertificate, NULL);
         _server->beginSecure("server.cer", "server.key", NULL);
     #else

code/platformio.ini

@@ -10,12 +10,13 @@ data_dir = espurna/data
 #   We use Arduino Core 2.3.0 (platformIO 1.5.0) as default
 #
 #   arduino core 2.3.0 = platformIO 1.5.0
-#   arduino core 2.4.0 = platformIO 1.6.0
-#   arduino core 2.4.1 = platformIO 1.7.3
-#   arduino core 2.4.2 = platformIO 1.8.0
-#   arduino core 2.5.0 = platformIO 2.0.4
-#   arduino core 2.5.1 = platformIO 2.1.1
-#   arduino core 2.5.2 = platformIO 2.2.2
+#   arduino core 2.4.0 = platformIO 1.6.0 (unsupported)
+#   arduino core 2.4.1 = platformIO 1.7.3 (unsupported)
+#   arduino core 2.4.2 = platformIO 1.8.0 (unsupported)
+#   arduino core 2.5.0 = platformIO 2.0.4 (unsupported)
+#   arduino core 2.5.1 = platformIO 2.1.1 (unsupported)
+#   arduino core 2.5.2 = platformIO 2.2.3 (unsupported)
+#   arduino core 2.6.1 = platformIO 2.3.0
 # ------------------------------------------------------------------------------
 arduino_core_2_3_0 = espressif8266@1.5.0
 arduino_core_2_4_0 = espressif8266@1.6.0