New TLS implementation #1520

Tico06 · 2022-05-10T13:09:48Z

Hi There,

I was not fully satisfied with the way SSL was implemented. Here is main point of my proposal:

You are now able to submit up to three Certificate Authorities to validate the mqtt broker certificate.
I made tests with Let's Encrypt and you may need to store three root Certificate Authorities to validate a server signed by them. Please see Chain of Trust - Let's Encrypt
You are now able to validate with the mqtt broker fingerprint if the previous point doesn't fit.
It's easier, but less secure and less convenient. While root Certificate Authorities are updated rarely (some are valid more than 10 years), the fingerprint should be updated each time the mqtt broker certificate is updated. With Let's Encrypt it's every quarter.
At last, if you don't have a root Certificate Authority nor the fingerprint, you are good to go with insecure connection.
The mqtt broker certificate is not validated. This setup is automatic if previous two setups are not done.
If required by the mqtt broker, you can setup a client certificate and key.
This is not mandatory, only if required by the mqtt broker.
If the connection to the mqtt broker fails, the _MQTT_ethClient.getLastSSLError is called and the SSL error is sent to the Debug console.

Happy to contribute, not big/complexe code, but was missing for me. Please get back to me for any questions, comments.

Regards,

Eric.

mfalkvidd · 2022-05-10T14:33:32Z

Many thanks for this contribution.

I'll add some specific review comments to the PR, but here are a few generic thoughts:

Technically, esp8266's Wificlientsecure only support TLS1.0 to TLS 1.2. It does not actually support any SSL version. Should we call stuff TLS instead of SSL, because it is technically correct, or should we stick with SSL because (maybe?) that's what people call it?

How will this change affect non-esp8266 sketches that use secure connection? Will https://github.com/mysensors/MySensors/blob/master/examples/GatewayW5100MQTTClient/GatewayW5100MQTTClient.ino still work? We wouldn't want to break people's sketches when they upgrade the MySensors library.

All new keywords must be added to https://github.com/mysensors/MySensors/blob/development/MyConfig.h and https://github.com/mysensors/MySensors/blob/development/keywords.txt
The former makes sure the documentation for the keywords is available at https://www.mysensors.org/apidocs-beta/index.html
The latter makes sure the keywords are highlighted in the Arduino IDE (and not highlighted when misspelled, which can be very useful).
The CI system usually helps pointing this out, but for some reason this PR has not been picked up by CI. I'll ask the team if someone can figure out why CI is broken. Edit: Here is the Butler report: https://ci.mysensors.org/job/MySensors/job/MySensors/view/change-requests/job/PR-1520/lastBuild/The_20Butler_20report/

mfalkvidd · 2022-05-10T14:13:11Z

examples/GatewayESP8266SecureMQTTClient/GatewayESP8266SecureMQTTClient.ino

+
+
+// import NodeManager library (a nodeManager object will be then made available)
+#include <MySensors_NodeManager.h>


Could we have an example that doesn't require NodeManager? MySensors is designed to not require any external libraries, which makes it possible to use all examples out of the box.

With the current code, the GatewayESP8266SecureMQTTClient will only compile if the user has also installed the NodeManager library.

Ok, I'll have a look on that.

mfalkvidd · 2022-05-10T14:15:20Z

MyConfig.h

+ *
+ * @endcode
+ */
+//#define MY_SSL_CERT_AUTH1


Would you mind expanding on the name change from MY_MQTT_xxx for MQTT settings, to the use of MY_SSL for some of the MQTT settings? There might be some logic behind that I don't see. Maybe the SSL stuff can be used for other things than MQTT? If that's the case, could we document hwo to use it for the other stuff?

Also, for all things renamed we'll need a strategy for backwards compatibility. Otherwise people's sketches will break when people upgrade the MySensors library and that will make people unhappy.

In fact, I started implementing on 2.3.2 without checking if an implementation was done in dev branch. What a surprise to me when I wanted to create a PR seeing it was implemented in dev since months... Well, hopefully my contribution goes a step forward, and still thinking it's valuable.

I'll have a look what I can do to stick with the naming and keep a compatibility layer between the both proposal.

Tico06 · 2022-05-10T15:57:22Z

Many thinks for this contribution.

I'll add some specific review comments to the PR, but here are a few generic thoughts:

Technically, esp8266's Wificlientsecure only support TLS1.0 to TLS 1.2. It does not actually support any SSL version. Should we call stuff TLS instead of SSL, because it is technically correct, or should we stick with SSL because (maybe?) that's what people call it?

Well.. From my side it's more natural to call it SSL. But yes, you right it's more TLS. If you are talking about the PR name, we can call it New TLS implementation. If you are talking about the constants naming, well I'll reply to your later comment on that.

How will this change affect non-esp8266 sketches that use secure connection? Will https://github.com/mysensors/MySensors/blob/master/examples/GatewayW5100MQTTClient/GatewayW5100MQTTClient.ino still work? We wouldn't want to break people's sketches when they upgrade the MySensors library.

Shouldn't affect no other hardware as all the code if filtered by a #ifdef (MY_GATEWAY_ESP8266_SECURE). I guess should work with an ESP32, but I don't have one in hand for the time being.

All new keywords must be added to https://github.com/mysensors/MySensors/blob/development/MyConfig.h and https://github.com/mysensors/MySensors/blob/development/keywords.txt The former makes sure the documentation for the keywords is available at https://www.mysensors.org/apidocs-beta/index.html The latter makes sure the keywords are highlighted in the Arduino IDE (and not highlighted when misspelled, which can be very useful). The CI system usually helps pointing this out, but for some reason this PR has not been picked up by CI. I'll ask the team if someone can figure out why CI is broken. Edit: Here is the Butler report: https://ci.mysensors.org/job/MySensors/job/MySensors/view/change-requests/job/PR-1520/lastBuild/The_20Butler_20report/

Ok, I'll have a look for the keywords, thx.

mfalkvidd · 2022-05-10T16:30:19Z

Great. The PR name doesn’t matter. Once the PR is merged the name will be ancient history :-) What’s important is the naming of the keywords, since they will live on in people’s sketches and in forum posts for a long time.

If there is anything you want to discuss, or would like help on, just post here. The community is usually very helpful.

mfalkvidd · 2022-05-10T16:33:05Z

Btw, if there is a clear advantage in deprecating existing keywords, see https://github.com/mysensors/MySensors/blob/development/MyConfig.h#L336 for an example of how we’ve maintained compatibility.

Tico06 · 2022-05-11T08:30:48Z

Great. The PR name doesn’t matter. Once the PR is merged the name will be ancient history :-) What’s important is the naming of the keywords, since they will live on in people’s sketches and in forum posts for a long time.

If there is anything you want to discuss, or would like help on, just post here. The community is usually very helpful.

Hi There !

I guess the Doxygen comments should be written in MyConfig.h, right ? The Butler report is not clear on this.

Thx & br,

Eric.

mfalkvidd · 2022-05-11T09:31:49Z

I guess the Doxygen comments should be written in MyConfig.h, right ? The Butler report is not clear on this.

Yes. Just follow the syntax of the existing documentation in MyConfig.h. I'll create an issue to amend the Butler code to be more clear.

Tico06 · 2022-05-12T12:11:48Z

Hi There,

Here are my updates, trying to follow all the recommendations above. The only deprecated is MY_MQTT_CA_CERT in favour to MY_MQTT_CA_CERT1.

When will the butler report be generated, and how to have access to it ?

Thanks and regards,

Eric.

Tico06 · 2022-05-13T07:42:53Z

Hi There,

The only issue left in the Butler report is "Body lines that are too wide (>72 characters)". After reading Amending older or multiple commit messages, it appears its strongly discouraged force pushing, which is required to amend an old commit. Should I ?

Or maybe I could revert/cancel the commit in my fork, close the PR and re-proceed ?

thanks and regards,

Eric.

anp369 · 2022-05-14T08:33:47Z

it appears its strongly discouraged force pushing, which is required to amend an old commit. Should I ?

Not an MySensors dev, but in the company I work for and from my personal experience, force-pushing is allowed or even intended on the topic branches, as usually there is only working one person on it. Force pushing usually becomes a problem when multiple people work on the same branch, as as mentioned in the guide completely changes the commit history for other people which would require them to fix it manually.

As this is your topic branch and it seems that only you are working on it, I don't see a problem force pushing to it.
The reviewer needs to "force pull" it to review it locally but that isn't a problem.

Implement TLS to mqtt server thanks to WiFiClientSecure class

Typo

Tico06 · 2022-05-14T11:09:15Z

Hi All,

Thanks a lot for your advices @anp369 & @mfalkvidd. The Butler report should be ok from now.

Regards,

Eric.

Move tls settings to bool gatewayTransportInit(void)

mfalkvidd · 2022-05-16T15:26:40Z

Nice.

The documentation is available at https://ci.mysensors.org/job/MySensors/job/MySensors/view/change-requests/job/PR-1520/5/execution/node/4/ws/MySensors/Documentation/html/group__GatewaySettingGrpPub.html

I'm unable to find documentation for MY_GATEWAY_ESP8266_SECURE but that could be just me looking in the wrong place.

Tico06 · 2022-05-16T18:09:11Z

Nice.

The documentation is available at https://ci.mysensors.org/job/MySensors/job/MySensors/view/change-requests/job/PR-1520/5/execution/node/4/ws/MySensors/Documentation/html/group__GatewaySettingGrpPub.html

I'm unable to find documentation for MY_GATEWAY_ESP8266_SECURE but that could be just me looking in the wrong place.

Hi There,

You're right... it's missing. I'll fix this.

BR

Eric.

mfalkvidd · 2022-05-19T08:15:45Z

Same error again. I don't understand why. Hopefully someone else in the team can help.

Tico06 · 2022-05-19T15:20:57Z

Hi There,

Well, the file exists. At least in my fork. But it looks for the file names 'PJONDefines.h:207' and 'PJONDefines.h:415'. Well that's what I understand from the log...

Thx and br

Eric.

Tico06 · 2022-05-31T14:49:08Z

Hi There,
Looks like we are stuck here. What are our options to progress on this ?
Thx and regards,
Eric.

mfalkvidd · 2022-06-02T07:27:28Z

Sadly enough, I have not been able to get help from the core team. I don't know how to proceed :(

tbowmo · 2022-06-02T08:19:42Z

I think we need the jenkins wizard @fallberg here.. If he has the time.. (I haven't played with jenkins myself, so I'm not sure what is going on there)

Tico06 · 2022-07-07T08:19:32Z

Hi There,

Rebased, remerged to benefit from the fix PR #1528

Tico06 · 2022-07-07T12:19:18Z

Hi There,

Looks like checking is stuck with the following message in console:

Trying to acquire lock on [arduinoEnv]
Found 0 available resource(s). Waiting for correct amount: 1.
[arduinoEnv] is locked by MySensors » MySensors » PR-1479 #16, waiting...

I had a look in #1479 and it looks completed.

don't have any clue on how to progress on this one.

thx and br

Eric.

mfalkvidd · 2022-07-07T13:32:39Z

Thanks for checking Eric. I logged in to the CI web gui. It showed PR #1333 as still running (with 8h run time so far). I stopped that build and it looks like #1520 is no longer stuck. Let's wait a bit to see if it completes.

mfalkvidd · 2022-07-07T15:06:22Z

Build fails on MySensorsMicro Examples. But I don't understand why those examples are active for the MySensorsMicro build.

One of the errors:

/var/lib/jenkins/jobs/MySensors/jobs/MySensors/branches/PR-1520/workspace/MySensors/core/MyGatewayTransportMQTTClient.cpp:119:1: error: 'BearSSL' does not name a type

MyGatewayTransportMQTTClient.cpp:119 is guarded by #if defined(MY_MQTT_CA_CERT1) but maybe that's not sufficient? Do we need to change to #if defined(MY_MQTT_CA_CERT1) && defined(MY_GATEWAY_ESP8266_SECURE) or something similar, to avoid compiling the example for non-esp platforms?

Tico06 · 2022-07-07T15:33:55Z

Thanks @mfalkvidd for looking at this. In fact, it's protected by #elif defined(MY_GATEWAY_ESP8266_SECURE). If you insert tabs in if/elif loops it looks like this:

#if defined(MY_GATEWAY_ESP8266) || defined(MY_GATEWAY_ESP32)
     #define EthernetClient WiFiClient
#elif defined(MY_GATEWAY_ESP8266_SECURE)
     #define EthernetClient WiFiClientSecure
     #if defined(MY_MQTT_CA_CERT1)
          BearSSL::X509List certAuth; //List to store Certificat Authorities
     #endif
     #if defined(MY_MQTT_CLIENT_CERT) && defined(MY_MQTT_CLIENT_KEY)
          BearSSL::X509List clientCert; //Client public key
          BearSSL::PrivateKey clientPrivKey; //Client private key
     #endif
     // Set time via NTP, as required for x.509 validation
     // BearSSL checks NotBefore and NotAfter dates in certificates
     // Thus an approximated date/time is needed.
     void setClock()
     {
             configTime(3 * 3600, 0, "pool.ntp.org", "time.nist.gov");

             Serial.print("Waiting for NTP time sync: ");
             time_t now = time(nullptr);
             while (now < 8 * 3600 * 2) {
                     delay(500);
                     Serial.print(".");
                     now = time(nullptr);
             }
            Serial.println("");
            struct tm timeinfo;
            gmtime_r(&now, &timeinfo);
            Serial.print("Current time: ");
            Serial.print(asctime(&timeinfo));
     }
#elif defined(MY_GATEWAY_LINUX)
// Nothing to do here
#else
uint8_t _MQTT_clientMAC[] = { MY_MAC_ADDRESS };
#endif /* End of MY_GATEWAY_ESPxy */

Looks like it tries to compile all MySensors examples. We should somehow avoid to compile examples for one platform with others. Eg: compile ESPxxx examples with ArduinoMicro platform. Maybe I missed something in my code.

BR

Eric.

Tico06 · 2022-07-07T15:37:51Z

In the checks list one is "Toll gate (ESP8266 - Examples) ". I guess my example should be ran only from here. How could I do that ?

Tico06 · 2022-07-07T15:59:58Z

I had a look in .ci dir. Looks like I should add a line in arduino.groovy to avoid platform cross compiling. I'll try this.

mfalkvidd · 2022-07-07T16:04:48Z

I don't know. But the list of examples for MySensorsMicro seems to be defined at

MySensors/.ci/arduino.groovy

Line 47 in 2e00bf6

def buildMySensorsMicro(config, sketches, String key) {

So examples/GatewayESP8266SecureMQTTClient/GatewayESP8266SecureMQTTClient.ino should not be included in MySensorsMicro. Therefore, MY_GATEWAY_ESP8266_SECURE should not be defined for MySensorsMicro and we should not get the error we are getting. Or am I missing something?

mfalkvidd · 2022-07-07T16:08:06Z

I thing I understand now. The list of files in arduino.groovy are files that should be excluded. So it should be sufficient to add examples/GatewayESP8266SecureMQTTClient/GatewayESP8266SecureMQTTClient.ino to the list?

Tico06 · 2022-07-07T16:10:53Z

Yep, that's also my understanding. Let's give it a try.... I'll commit.

mfalkvidd · 2022-07-07T19:31:29Z

Warning:

In file included from /var/lib/jenkins/jobs/MySensors/jobs/MySensors/branches/PR-1520/workspace/MySensors/examples/GatewayESP8266SecureMQTTClient/GatewayESP8266SecureMQTTClient.ino:73:0:
/var/lib/jenkins/jobs/MySensors/jobs/MySensors/branches/PR-1520/workspace/build/sketch/certs.h:10:1: warning: multi-line comment [-Wcomment]

mfalkvidd · 2022-07-07T19:34:25Z

Maybe changing to /* style comment helps?

mfalkvidd · 2022-07-08T08:16:07Z

Finally. Thanks everyone!

Tico06 · 2022-07-08T10:37:06Z

Yes ! Done ! Happy to have contributed ! Thank you all for your support and warm thanks to @mfalkvidd

* New TLS implementation Implement TLS to mqtt server thanks to WiFiClientSecure class * New TLS implementation Implement TLS to mqtt server thanks to WiFiClientSecure class * New TLS implementation Implement TLS to mqtt server thanks to WiFiClientSecure class * New TLS implementation Implement TLS to mqtt server thanks to WiFiClientSecure class * Update MyConfig.h Typo * Update GatewayESP8266SecureMQTTClient.ino Typo * MyGatewayTransportMQTTClient.cpp updated Move tls settings to bool gatewayTransportInit(void) * MySensors code styling applied by GIT * Try to fix Doxygen warnings * Doxygen warnings fixed hopefuly * MY_GATEWAY_ESP8266_SECURE doc added * MY_GATEWAY_ESP8266_SECURE doc completed * Avoid platform cross compiling * Replaced spaces indent by tabs * Multilines comments to /*

mfalkvidd reviewed May 10, 2022

View reviewed changes

Tico06 changed the title ~~New SSL implementation~~ New TLS implementation May 10, 2022

Tico06 closed this May 13, 2022

Tico06 reopened this May 13, 2022

New TLS implementation

dc43fc3

Implement TLS to mqtt server thanks to WiFiClientSecure class

Tico06 force-pushed the development branch from 0b60bb2 to 560b97a Compare May 14, 2022 11:01

Tico06 added 5 commits May 14, 2022 13:04

New TLS implementation

a32f865

Implement TLS to mqtt server thanks to WiFiClientSecure class

New TLS implementation

9d15c9b

Implement TLS to mqtt server thanks to WiFiClientSecure class

New TLS implementation

3d76128

Implement TLS to mqtt server thanks to WiFiClientSecure class

Update MyConfig.h

faf0206

Typo

Update GatewayESP8266SecureMQTTClient.ino

70eb7eb

Typo

Tico06 force-pushed the development branch from 560b97a to 8581ab6 Compare May 14, 2022 11:06

Tico06 added 3 commits May 15, 2022 08:50

MyGatewayTransportMQTTClient.cpp updated

181e56e

Move tls settings to bool gatewayTransportInit(void)

MySensors code styling applied by GIT

4b0943c

Try to fix Doxygen warnings

e051c67

Tico06 force-pushed the development branch from 8581ab6 to e051c67 Compare May 15, 2022 06:52

Doxygen warnings fixed hopefuly

b4a7123

MY_GATEWAY_ESP8266_SECURE doc added

be4785e

mfalkvidd requested a review from Yveaux May 19, 2022 08:14

virtual-maker mentioned this pull request Jul 1, 2022

Add CAN transport layer #1488

Open

Tico06 mentioned this pull request Jul 4, 2022

Update MyTransportPJON.cpp #1526

Merged

virtual-maker mentioned this pull request Jul 6, 2022

Replace memset() in PJONDefines.h to avoid build warning (#1527) #1528

Merged

Merge branch 'mysensors:development' into development

e04a444

Tico06 added 2 commits July 7, 2022 18:19

Avoid platform cross compiling

4070107

Replaced spaces indent by tabs

3700cc9

Multilines comments to /*

ba7a591

mfalkvidd added the release-notes Issues that have information that should be included in the release notes label Jul 8, 2022

mfalkvidd merged commit 97a70a1 into mysensors:development Jul 8, 2022



		// import NodeManager library (a nodeManager object will be then made available)
		#include <MySensors_NodeManager.h>

New TLS implementation #1520

New TLS implementation #1520

Conversation

Tico06 commented May 10, 2022 • edited Loading

mfalkvidd commented May 10, 2022 • edited Loading

mfalkvidd May 10, 2022

Choose a reason for hiding this comment

Tico06 May 10, 2022

Choose a reason for hiding this comment

mfalkvidd May 10, 2022

Choose a reason for hiding this comment

Tico06 May 10, 2022

Choose a reason for hiding this comment

Tico06 commented May 10, 2022

mfalkvidd commented May 10, 2022

mfalkvidd commented May 10, 2022

Tico06 commented May 11, 2022

mfalkvidd commented May 11, 2022

Tico06 commented May 12, 2022

Tico06 commented May 13, 2022 • edited Loading

anp369 commented May 14, 2022

Tico06 commented May 14, 2022

mfalkvidd commented May 16, 2022

Tico06 commented May 16, 2022

mfalkvidd commented May 19, 2022

Tico06 commented May 19, 2022

Tico06 commented May 31, 2022

mfalkvidd commented Jun 2, 2022

tbowmo commented Jun 2, 2022

Tico06 commented Jul 7, 2022

Tico06 commented Jul 7, 2022

mfalkvidd commented Jul 7, 2022

mfalkvidd commented Jul 7, 2022

Tico06 commented Jul 7, 2022 • edited Loading

Tico06 commented Jul 7, 2022

Tico06 commented Jul 7, 2022

mfalkvidd commented Jul 7, 2022

mfalkvidd commented Jul 7, 2022

Tico06 commented Jul 7, 2022

mfalkvidd commented Jul 7, 2022

mfalkvidd commented Jul 7, 2022

mfalkvidd commented Jul 8, 2022

Tico06 commented Jul 8, 2022

Tico06 commented May 10, 2022 •

edited

Loading

mfalkvidd commented May 10, 2022 •

edited

Loading

Tico06 commented May 13, 2022 •

edited

Loading

Tico06 commented Jul 7, 2022 •

edited

Loading