circuitpython: minimqtt lib problem with TLS broker
CircuitPython version
CP800b6 daily
Adafruit CircuitPython 8.0.0-beta.6-43-g1c1cf1cf5 on 2023-01-18; Raspberry Pi Pico W with rp2040
Board ID:raspberry_pi_pico_w
Code/REPL
negative number of bytes to read: -115
Behavior
use: CP800b6 latest ( from 14. and 18. ) ( with bundle /lib/ from 15. and 17. ) on a PICO_W ( nuked ) to connect to a local broker ( RPI mosquitto) OK BUT to a remote broker TLS ( HIVEMQ ) ERROR always get error at subscribe or publish about negative number of bytes to read: -115
i think that comes from a recent change of minimqtt lib
++ confirm, all same but replace /lib/minimqtt/ from adafruit-circuitpython-bundle-8.x-mpy-20221221.zip still works
Description
No response
Additional information
adafruit-circuitpython-bundle-8.x-mpy-20221221.zip /lib/minimqtt/ OK
About this issue
- Original URL
- State: closed
- Created a year ago
- Comments: 42 (4 by maintainers)
Commits related to this issue
- raspberrypi: SSLSocket: raise OSError when appropriate Rather than returning the negative error value. This is intended to close #7606, though I did not test with mqtt. Instead, I created a simple s... — committed to jepler/circuitpython by jepler a year ago
- Revert "Correctly raise OS error in socketpool_socket_recv_into()" This reverts commit 7e6e824d5655026906b6515070aeb604d2ef3426. Fixes #7770 The change in #7623 needs to be revered; the raise-site ... — committed to jepler/circuitpython by jepler a year ago
INSECURE means plaintext MQTT. If you want TLS, you need to set is_ssl to True.
On Wed, Feb 22, 2023, at 09:15, KLL wrote:
@kllsamui , the value of 100 I used was based on the two MQTT brokers I had available to test (1 local, 1remote). Some sites could require even more time. The fix in PR #7623 should take care of the problem regardless of the keep_alive timeout.
Testing (with a lot of tracing) has shown that the 116 is, in fact, an MP_ETIMEDOUT error code being returned from lwip_tcp_receive in common-hal/socketpool/Socket.c. I am unsure why the value is 116, as MP_ETIMEDOUT is defined as 110 in py/mperrno.h. Adding a “keep_alive=100” to the MQTT initialization in the supplied script avoids the timeout in my testing. There may well be some issue here that is keeping the error from being properly reported, perhaps because at some point the return value is being passed back up the chain as an unsigned.
Looking into the code of
subscribe()
, I can see potential for breakage: https://github.com/adafruit/Adafruit_CircuitPython_MiniMQTT/blob/342b8c99b2c8f10b605cc8311bf5f394f4d18e92/adafruit_minimqtt/adafruit_minimqtt.py#L785-L802The first issue is that the client expects SUBACK to be sent in direct response to SUBSCRIBE. If any other message type is received from (buggy/rogue) broker, the parsing will get out of sync, because part of the message will remain in the read buffer. In such (rare ?) case the other message should be processed fully or exception thrown.
The second issue is that the code expects fixed length of the SUBACK, specifically 4 bytes (L788 above), possibly assuming single topic to be subscribed. When a client subscribes to multiple topics at once (which the
subscribe()
function allows), the SUBACK will contain return codes for each topic.Unsubcribe handling has the same problem, however this will matter only for MQTT protocol version 5 and above, I believe.
There are 2 possibilities I can see how the
to_read
variable in_sock_exact_recv()
can become negative:bufsize
parameter of the function was already negativerecv_len
returned fromrecv_into()
was strictly bigger than thebufsize
The former might be a problem in MQTT message parsing, the latter would be a bug in CircuitPython core. Now, there are only 2 calls to
_sock_exact_recv()
that supply non-literal number and these are both in MQTT PUBLISH message parsing.If you are willing to help debug this, I will prepare a special version of
adafruit_minimqtt.py
with extra debug logging that can be placed on the Pico W and be used to shed more light on this issue. In the mean time, installadafruit_logging
and enable logging with:I will begin from the end. The
is_ssl
parameter as it is implemented currently, is set toTrue
by default and as long as the port is set to 8883 and thessl_context
parameter is valid, TLS will be used. There is no fallback to insecure (plaintext) connection if TLS setup fails (which is a good thing as it avoids downgrade attacks). The default behavior ofis_ssl
might or might not change with PR adafruit/Adafruit_CircuitPython_MiniMQTT#145. Anyhow, if TLS is desired, settingis_ssl
toTrue
explicitly is the right thing to do, even though it is not needed currently.I registered an account at HiveMQ and extracted relevant code from
code.py
however cannot replicate the problem with the latest minimqtt module, at least using CPython on MacOS - the subscribe goes through just fine.