eclipse / paho.mqtt.python

paho.mqtt.python
Other
2.17k stars 723 forks source link

set_tls: incorrect default tls_version? #668

Open wosym opened 2 years ago

wosym commented 2 years ago

We are currently investigating an issue in our software, and came across this weird construction that might or might not be related to the problems we're experiencing.

In v1.6.1 the documentation says the following about the tls_version parameter:

tls_version allows the version of the SSL/TLS protocol used to be specified. By default TLS v1.2 is used. Previous versions are allowed but not recommended due to possible security problems.

So... naturally what I would assume is that if I don't pass this parameter, it would default to TLSv1.2. However, in practice we're seeing different behaviour when passing this default explicitly, and not passing it at all. This can be explained by the code that handles the setting of the default:

        if tls_version is None:
            tls_version = ssl.PROTOCOL_TLSv1_2
            # If the python version supports it, use highest TLS version automatically
            if hasattr(ssl, "PROTOCOL_TLS"):
                tls_version = ssl.PROTOCOL_TLS

So indeed, first the default is set to ssl.PROTOCOL_TLSv1_2, but afterwards it is overridden again by ssl.PROTOCOL_TLS! I think the documentation in its current state is very misleading, since it does not indicate this. Similar confusing statements were present in previous versions also.

A second problem with this default value that I noticed is that the comment says it will use the highest version automatically. However, I noticed that ssl.PROTOCOL_TLS resolves to int 2. This is the same as PROTOCOL_SSLv23, which is definitely not the highest version available. Based on #653, I tried changing ssl.PROTOCOL_TLS to ssl.PROTOCOL_TLS_CLIENT. If I do that, the int resolves to int 16. I do not know with what enum this corresponds (couldn't find it. It's also not known to Python PDB), but at least it sounds like that makes more sense?

Is this a bug? Or is this working as intended? And if it's a bug, is it a bug in paho-mqtt, or in openssl-python?

MattBrittan commented 8 months ago

Agreed; looks like a bug (at a minimum the docs need to match the code).