eclipse / paho.mqtt.python

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

Defaults for tls={'ca_certs': ...} #798

Open runout-at opened 6 months ago

runout-at commented 6 months ago

Running on Debian bookworm, Python 3.11 i found that tls dictionary is missing a default for ca_certs.

I would expect the default is /etc/ssl/certs/ca-certificates.crt in Debian.

Following works at least under Debian: tls={'ca_certs':"/etc/ssl/certs/ca-certificates.crt"}

Note: Originally posted as a comment on issue #518

MattBrittan commented 6 months ago

Sorry - please clarify your specific issue (i.e. "The following code fails to run on debian stable (bookworm) with the error ..."). The text copied from the previous issue is confusing (its fine for an issue to reference another issue but it should also stand alone).

runout-at commented 6 months ago

Running on Debian bookworm, Python 3.11 i found that tls dictionary is missing a default for _cacerts.

I would expect the default is /etc/ssl/certs/ca-certificates.crt in Debian.

Following works at least under Debian: tls={'ca_certs':"/etc/ssl/certs/ca-certificates.crt"}

runout-at commented 6 months ago

Sorry, creating a new issue from the old one messed something up.

runout-at commented 6 months ago

I have no access to the system right now. I will reinvestigate in some days as this is an ARM architecture.

Couldn't reproduce it on AMD64 right now.

thx @MattBrittan for your work!

runout-at commented 6 months ago

Yesterday I just tried a subscribe which did work. The problem occurs only on publishing and I did look at the code now.

The doc inside the code of publish.simple and publish.multiple for tls says _cacerts is required. I didn't find this in the docomentation or missed it. The default is tls=None but if _cacerts is required inside the tls dict, None does not make much sense.

But if you invoke these functions with tls={"ca_certs": "proper path to cert"} or even only tls={} as parameter it seems to work. The latter I didn't expect to work from the inline docu.

The code for that is around line 164 in paho/mqtt/publish.py:

    if tls is not None:
        if isinstance(tls, dict):
            insecure = tls.pop('insecure', False)
            client.tls_set(**tls)
            if insecure:
                # Must be set *after* the `client.tls_set()` call since it sets
                # up the SSL context that `client.tls_insecure_set` alters.
                client.tls_insecure_set(insecure)
        else:
            # Assume input is SSLContext object
            client.tls_set_context(tls)

    client.connect(hostname, port, keepalive)
    client.loop_forever()

I think the first if should have an else. Otherwise we would end in an endless loop.

Consider following example to reproduce this:

import paho.mqtt.publish as publish

host = "mqtt.example.com"
username = "user@mqtt.example.com"
password = "password of user"
topic = "test/topic1"

ca_certs = "/etc/ssl/certs/ca-certificates.crt"
tls = {'ca_certs': ca_certs}

print('with tls:',
      publish.single(topic, "payload",
                     hostname=host, port=8883,
                     auth={'username':username, 'password':password},
                     client_id="mqtt_pub_1",
                     tls={}
                     )
      )

print('with tls:',
      publish.single(topic, "payload",
                     hostname=host, port=8883,
                     auth={'username':username, 'password':password},
                     client_id="mqtt_pub_1",
                     tls=tls
                     )
      )

print('without tls:',
      publish.single(topic, "payload",
                     hostname=host, port=8883,
                     auth={'username':username, 'password':password},
                     client_id="mqtt_pub_1"
                     )
      )

this prints

with tls: None
with tls: None

The first 2 messages are properly received by the mqtt server and then the publishing client hangs in the loop on the third call of publish.simple

runout-at commented 6 months ago

Maybe it would be enough to set tls variable if it's None inside publish.multiple like

    if tls is None:
       tls = {}

and remove the

    if tls is not None:

(and remove one indent level of the following if)

PierreF commented 6 months ago

It's wanted to have tls=None for the default. That the means not using TLS but clear-text which is the default and a supported behavior.

Your third client "hanging" is actually trying to connect (in a loop) to a broken that reject its connection; because client/server protocol don't match. I'm not sure we can do much easily. We don't have easy way to known this issue will be persistent.

There is indeed the documentation / code documentation / code implementation that are not always very well aligned. For instance here:

I've in my mind to review documentation, probably stop to have both code documentation and documentation that repeat. But that a significant amoung of work.

For your case, you can rely on tls={} to work and stay being supported. I think I'll add support for tls=True and tls=False (and use False for the default) which seems less confusing than tls={} and tls=None.

runout-at commented 6 months ago

It's wanted to have tls=None for the default. That the means not using TLS but clear-text which is the default and a supported behavior.

thx. for clarification.

Your third client "hanging" is actually trying to connect (in a loop) to a broken that reject its connection; because client/server protocol don't match. I'm not sure we can do much easily. We don't have easy way to known this issue will be persistent.

It would be great f it could have a timeout or retry limit and give some informative error message.

There is indeed the documentation / code documentation / code implementation that are not always very well aligned. For instance here:

* tls don't require any key, and empty dict is indeed supported (it wasn't supported 7 years ago, I think that from there this documentation come from).

* tls could be a TLSContext, which is not documented at all.

I did read about TLSContext in the docs for subscription to a broker but did'n't know how to use it.

I've in my mind to review documentation, probably stop to have both code documentation and documentation that repeat. But that a significant amoung of work.

I can imagine!

For your case, you can rely on tls={} to work and stay being supported. I think I'll add support for tls=True and tls=False (and use False for the default) which seems less confusing than tls={} and tls=None.

That's great to know and I agree that tls=False would be more intentional than tls=None.

Thx a lot!