Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
runout-at opened this issue Jan 8, 2024 · 8 comments
Open

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

runout-at opened this issue Jan 8, 2024 · 8 comments
Labels
Status: Available No one has claimed responsibility for resolving this issue. Type: Enhancement A new feature for a minor or major release.

Comments

@runout-at
Copy link

runout-at commented Jan 8, 2024

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

@github-actions github-actions bot added the Status: Available No one has claimed responsibility for resolving this issue. label Jan 8, 2024
@runout-at runout-at changed the title @runout-at there were around 300 open issues dating back many years; many of which had no responses at all. I'm attempting to get things into a manageable state so that issues can be categorised/resolved in a reasonable time-frame (as things stand the issue backlog is very intimidating, so has been largely ignored; resources are very limited). As you can probably understand working through this number of issues takes a considerable amount of time, and I will make mistakes (and am deliberately being somewhat ruthlessness through necessity). Defaults for tls={'ca_certs': ...} Jan 8, 2024
@MattBrittan
Copy link
Contributor

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).

@MattBrittan MattBrittan added the Status: More info needed More information needed from issue author label Jan 8, 2024
@runout-at
Copy link
Author

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"}

@runout-at
Copy link
Author

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

@runout-at
Copy link
Author

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
Copy link
Author

runout-at commented Jan 9, 2024

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 ca_certs is required. I didn't find this in the docomentation or missed it. The default is tls=None but if ca_certs 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
Copy link
Author

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
Copy link
Contributor

PierreF commented Jan 9, 2024

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:

  • 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'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.

@PierreF PierreF added Type: Enhancement A new feature for a minor or major release. and removed Status: More info needed More information needed from issue author labels Jan 9, 2024
@runout-at
Copy link
Author

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Available No one has claimed responsibility for resolving this issue. Type: Enhancement A new feature for a minor or major release.
Projects
None yet
Development

No branches or pull requests

3 participants