New TLS implementation - Githubissues

Tico06 commented 2 years ago

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 commented 2 years ago

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/

Tico06 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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.

Tico06 commented 2 years ago

Hi All,

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

Regards,

Eric.

mfalkvidd commented 2 years ago

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 commented 2 years ago

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.

Tico06 commented 2 years ago

Hi There,

Not sure I catch the point of the 'Toll gate (STM32F1 - Tests)' errors:

Is the file '/var/lib/jenkins/jobs/MySensors/jobs/MySensors/branches/PR-1520/workspace/MySensors/hal/transport/PJON/driver/PJONDefines.h:207' a valid filename? Is the file '/var/lib/jenkins/jobs/MySensors/jobs/MySensors/branches/PR-1520/workspace/MySensors/hal/transport/PJON/driver/PJONDefines.h:415' a valid filename?

Or should I look in another direction ?

Thanks and regards,

Eric.

mfalkvidd commented 2 years ago

Hi There,

Not sure I catch the point of the 'Toll gate (STM32F1 - Tests)' errors:

Is the file '/var/lib/jenkins/jobs/MySensors/jobs/MySensors/branches/PR-1520/workspace/MySensors/hal/transport/PJON/driver/PJONDefines.h:207' a valid filename? Is the file '/var/lib/jenkins/jobs/MySensors/jobs/MySensors/branches/PR-1520/workspace/MySensors/hal/transport/PJON/driver/PJONDefines.h:415' a valid filename?

I don't understand much of what happens in Jenkins, but the logs show the following:

java.io.IOException: Failed to copy /var/lib/jenkins/jobs/MySensors/jobs/MySensors/branches/PR-1520/workspace/MySensors/hal/transport/PJON/driver/PJONDefines.h:207 to /var/lib/jenkins/jobs/MySensors/jobs/MySensors/branches/PR-1520/builds/6/workspace-files/fe1411a2.tmp
 Caused by: java.nio.file.NoSuchFileException: /var/lib/jenkins/jobs/MySensors/jobs/MySensors/branches/PR-1520/workspace/MySensors/hal/transport/PJON/driver/PJONDefines.h:207

It seems strange that Jenkins tries to copy a file that does not exist. I have triggered a new build, let's see if that helps.

mfalkvidd commented 2 years ago

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

Tico06 commented 2 years ago

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 commented 2 years ago

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

mfalkvidd commented 2 years ago

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

tbowmo commented 2 years ago

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 commented 1 year ago

Hi There,

Rebased, remerged to benefit from the fix PR #1528

Tico06 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

I don't know. But the list of examples for MySensorsMicro seems to be defined at https://github.com/mysensors/MySensors/blob/2e00bf6a10f76d6aaa1999e12313237bc3edabd3/.ci/arduino.groovy#L47

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 commented 1 year ago

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 commented 1 year ago

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

mfalkvidd commented 1 year ago

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 commented 1 year ago

Maybe changing to /* style comment helps?

mfalkvidd commented 1 year ago

Finally. Thanks everyone!

Tico06 commented 1 year ago

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

mysensors / MySensors

New TLS implementation #1520