Skip to content

Conversation

@diegopx
Copy link

@diegopx diegopx commented Mar 3, 2017

A new constructor (available only on the ESP8266) allows the user to verify the SSL certificate fingerprint. Also, to avoid read/write collisions in axTLS buffer, mqtt.loop processes all incoming messages instead of just one.

diegopx added 2 commits March 3, 2017 17:01
A new constructor allows specifying the server's SSL certificate's
fingerprint, which is used to verify the server's identity after
the client is securely connected.

It only works on the ESP8266, which has the computation power and
memory necessary to do SSL in practice, using WiFiClientSecure.
The MQTT loop now processes all incoming messages at once, instead
of stopping at the first one. For the HTTPS client, this ensures
the axTLS's read buffer has been completely useed before exiting
the function, leaving it available for writing in any subsequent
publish() calls.
@knolleary
Copy link
Owner

Thanks for this. Don't know how soon I'll be able to look at it properly to get it merged. Have you got a link to the relevant ESP8266 docs for the TLS support?

One observation; the client allows publishing from the message-arrived callback. According to the comments you've added, doing so would trash the axTLS buffer and you'd lose messages?

@diegopx
Copy link
Author

diegopx commented Mar 3, 2017

In their master as it is now (esp8266/Arduino@04df3ad), yes, trying to publish inside the message handler would trash the buffer and any further read would return garbage. Then, what usually happens is PubSubClient halts (or times out) because the read message length is a arbitrary large number and the socket will wait to get that many bytes.

I've fix that in my fork of Arduino, so WiFiClientSecure::write() returns an error instead of trashing the buffer (see pull request esp8266/Arduino#3019). Hence, mqtt.publish() might fail inside the message callback.
If there were no further messages on the SSL socket when reading the PUBLISH message packet, then the publish() would succeed as usual.

Since WiFiClientSecure uses axTLS, a general never-fail publish-in-onmessage is not possible without an additional buffer, because two messages could come in the same SSL packet, and the first one will never be able to publish without killing the buffer for the second.

@diegopx
Copy link
Author

diegopx commented Mar 4, 2017

As a sidenote, one disadvantage of the code is that since it loops reading messages, there is a possible case where the client is overflown with new messages and mqtt.loop() never returns, so the rest of the user code never executes. Although this situation is very extreme, it may still happen.

Only axTLS is half duplex, the internal ClientContext has its own (non-decrypted) buffer. So, the collisions only start when doing ssl_read(), which reads from the ClientContext into its own buffer and decrypts (i.e. the collisions do not start immediately when the data arrives to the device). Therefore, there are moments where there may still be a non-decrypted message in the ClientContext but the axTLS buffer is empty. These moments are ideal to exit from the loop.
In particular, calling WiFiClientSecure.available() gives the exact size remaining in the buffer, but induces a new ssl_read() if the buffer was empty. So I'll try and see if I can reduce the number of available() calls and caching such value so that the processing only consumes the "original buffer" (i.e. the buffer as of the moment mqtt.loop() was called) and does not keep on decrypting further messages. This way the rest of the user loop has a chance to run.

The MQTT loop processes all messages to avoid having buffer
collisions on axTLS. However, this introduces the possibility of
an infinite loop because of heavy inbound traffic, giving no chance
to the rest of the user code to run.

A new availability cache has been introduced to avoid inducing buffer
reads if possible. If after a message has been processed the buffer
looks empty, the loop will exit (even if there is information in a
secondary buffer or the real non-cached availability has grown).

The only way to trap the loop forever now would be to receive a
continuous stream of "misaligned" messages which never end on
buffer boundaries.
New functionality used C++11 in-class initialization. It has been
replaced with initialization in the constructor.
@odelot
Copy link

odelot commented Aug 16, 2017

hi @diegopx. btw did you get this behavior while using WifiClientSecure with the mqtt server?

The WifiClientSecure.write method returns with success even though there is no internet connection? (so the device is connect to a router, sending data to a clouded mqtt server and even with no internet it returns success)

This behaivor leads to a memory problem and freezes the device after a while (the WifiClientSecure continues to allocate memory to encrypt the message even it does not send it).

@lizaoreo
Copy link

Has this not been merged?

@dbudwin
Copy link

dbudwin commented Feb 26, 2018

@knolleary Any progress on this? I really like this library and would like to be able to continue using it to securely subscribe to MQTT topics.

@Enrico204
Copy link

@diegopx I noticed that you're using verify before checking for result variable to be equal to 1. I think that this might lead to crash when connect fails (I just tested it, and it seems to crash when MQTT server is not responding).

@diegopx
Copy link
Author

diegopx commented Feb 27, 2018

It's been a while, but from what I can see, you're correct; the connect return value should be checked before verifying.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants