SuperHouse / esp-open-rtos

Open source FreeRTOS-based ESP8266 software framework
BSD 3-Clause "New" or "Revised" License
1.53k stars 491 forks source link

extras/paho_mqtt_c Outdated #470

Open jeffsf opened 6 years ago

jeffsf commented 6 years ago

In the process of trying to use the mbedTLS library with the paho_mqtt_c library, I found that the paho_mqtt_c library is on the order of two years out of date. As security is the driver behind this work, I would much prefer to have a more-current version of the paho libraries in use.

While the mbedTLS library seems to have minimal changes with respect to https://github.com/ARMmbed/mbedtls the paho_mqtt_c library is substantially different than https://github.com/eclipse/paho.mqtt.embedded-c even on a file-by-file basis. I am guessing that a significant number of the differences may be due to the esp-open-rtos build system and a likely requirement that the #define-s and symbol names must be unique.

Going on that assumption, to be able to maintain the port of paho.mqtt.embedded-c to paho_mqtt_c, a script or set of scripts would be very useful that would

A script of this type may be useful for other "extras" with upstream source.

Is my assumption about the need for "globally" unique #define-s and symbol names correct?

Does such a script already exist, in part or in whole?

Once I get the current paho_mqtt_c working with TLSv1.2 I may take a stab at it.

jeffsf commented 6 years ago

Well, I've taken a good stab at comparing v1.0.0 upstream having found over 140 define/variable-name changes and it's somewhat recognizable. There's also a healthy dose of formatting style changes that will make a "clean" merge close to impossible.

I'd suggest a re-port of the upstream code, especially given that it has apparently has already been ported to FreeRTOS upstream. https://github.com/eclipse/paho.mqtt.embedded-c/tree/master/MQTTClient-C/src/FreeRTOS

kanflo commented 6 years ago

Good catch. I am hoping someone really in need of MQTT for EOR will do the porting and raise a PR. At best, the upstream code could be included as a submodule with additional glue as needed. Eg. SPIFFS is included this way.

jeffsf commented 6 years ago

I did note that the mbedTLS libraries consist of a "wrapper" and then a "stock" checkout of the upstream source. That seems like a good pattern to me.

Is this project "religious" about putting *.h identifiers into their own namespaces, or is conflict resolution (if it even occurs) generally left to the user? If the first, is there an established naming practice? My preference would be to leave them alone, as much as possible.

I'm working through why what is running with "stock" mbedTLS and MQTTClient-C on my desktop is doing all kinds of strange things on my ESP8266; mbedtls_entropy_init() hanging, known-good cert not verifying, ... If it takes much longer, the port of MQTTClient-C may rise even more quiclky to the top of my list. The recent CVE on WPA2 vulnerability further reinforces the need for TLSv1.2 transport, even if just to protect the MQTT server.

_Edit: For anyone reading this in the future, perhaps in search of resolution for their own challenge, the problem with entropy was not the mbedTLS library or directly an esp-open-rtos problem. The memset of the 512-byte entropy buffer apparently caused stack overflow in a way that was not caught by #define configCHECK_FOR_STACK_OVERFLOW 2_

jeffsf commented 6 years ago

Still working on getting a "reference implementation" going with the current mbedTLS and MQTTClient-C. Tracking down why mbedTLS isn't able to verify certificates under esp-open-rtos, though the same code compiled on a POSIX platform is successful (yes, lots of "fake" *.h and function stubs needed). I'm guessing at this point that it is either the lack of a time reference, or something buggy in the math libraries.

Once I have a reference implementation working, I'll be taking a stab at porting the upstream MQTTClient-C FreeRTOS code to esp-open-rtos.

For anyone trying to search on this:

! The certificate is not correctly signed by the trusted CA

ssl_tls.c:4587: |1| x509_verify_cert() returned -9984 (-0x2700)

Edit: Time was a distraction, but a helpful one as adding changed memory allocation, which changed the error messages. I'm getting "valid" from the cert now in a narrow band of task heap size, even with the time at the epoch. Too large of a task heap and get all kinds of error messages, some of which have nothing to do with memory allocation

jeffsf commented 6 years ago

Making reasonable progress on the port of upstream https://github.com/eclipse/paho.mqtt.embedded-c So far it looks like I should be able to do it without changing the upstream source. One local #define at a time...

Thanks to @ourairquality for getting upstream to resolve http://savannah.nongnu.org/bugs/?52190

jeffsf commented 6 years ago

Unfortunately, the upstream platform-specific code for FreeRTOS relies on blocking read and it appears that socket timeouts are not supported with LWIP. #477

At least we'll get the current upstream source. Working on writing a select()-based approach next.

ourairquality commented 6 years ago

@jeffsf Socket timeouts should be working on lwip. Using the FreeRTOS socket api might be the problem, so perhaps see if there is posix style socket support.

jeffsf commented 6 years ago

Port is "works for me" now and builds without unintentional warnings with -Wall.

There is an intentional #warning inserted about lwip_gethostbyname() not being thread-safe. If anyone reading this can point me to a memory-efficient, thread-safe way to resolve a host name with LWIP, I'd appreciate it. The documentation for lwip_gethostbyname_r() is pretty sketchy, such as determining the size of the buffer needed. I don't see HOSTENT_STORAGE defined in esp-open-rtos.

I've got on-the-fly patching of upstream sources tied into the build system, all done under $(BUILD_DIR)

Patches are very minimal; missing header #include here and there, defining a function in a header (multiple versions of the same symbol in the esp-open-rtos build system), and, surprisingly, the upstream FreeRTOS-specific code not checking for EAGAIN.

Working through README.md and licensing. Three-Clause BSD OK with the project?

ourairquality commented 6 years ago

There does not appear to be provision in newlib for thread load storage for gethostbyname. Perhaps it is just assumed that gethostbyname_r will be used and I suggest using that. Looking at the source code in netdb.c suggests the buffer needs to be at least (sizeof(struct gethostbyname_r_helper) + LWIP_MEM_ALIGN_BUFFER(namelen + 1)).

jeffsf commented 6 years ago

Of course struct gethostbyname_r_helper is internal and not present in lwip/netdb.h :unamused:

Quick check from my running board (IPv4 only, I believe, as it determines the sizeof(ip_addr_t)) gives

128    DNS_MAX_NAME_LENGTH
177    NETDB_ELEM_SIZE
148    sizeof(struct gethostbyname_r_helper) + \ 
              LWIP_MEM_ALIGN_BUFFER(DNS_MAX_NAME_LENGTH + 1)

Thankfully not so big as to warrant fancy memory management. Edit: Unfortunately, what it points to would need to be freed, so I'll probably end up using getaddrinfo rather than rewrite the equivalent of freeaddrinfo

So as not to blindly assume that is the maximum, the function is linear with size

lwip/arch.h:#define LWIP_MEM_ALIGN_BUFFER(size) (((size) + MEM_ALIGNMENT - 1U))
jeffsf commented 6 years ago

https://github.com/jeffsf/extras_paho-embed-mqtt3c

I wouldn't say it's "pull ready" especially as it doesn't include a working example, but it seems pretty solid to me.

Has the platform-specific MQTTSimple.h and MQTTSimple.c which are built on LWIP calls, and uses getaddrinfo to get around the potential problems with gethostbyname

No "shim" included because FreeRTOS is GPL and there is yet to be any legal consensus around copyright and license of APIs and function prototypes.

Edit: The welcome change of FreeRTOS to a compatible license means I'll be able to pick this up again soon

avrhack commented 6 years ago

Jeff I was just about to start doing the exact same thing - in my case because QOS2 simply doesn't work with the version of PAHO that's in esp-open-rtos. However it looks like you've done all the hard work already, just not quite got it over the line.

How can I help turn this into a PR so we can get it into the baseline code as I'm sure it's not just you and I who would benefit from this?

jeffsf commented 5 years ago

Just getting back to dusting this off, hopefully with the ability to use mbedTLS as transport as well. I was holding off to make sure that the APIs and build approach were going to support that. No guarantees on availability of time, but I'm still around!