Add the possibility to use tls_set() in addition to tls_context about aiomqtt HOT 10 CLOSED

Hard to disagree with your thoughts...

I'll see if I can follow your design suggestions and come up with something.

Alex

from aiomqtt.

Good thinking. :) I like the idea of having this functionality as a free function (your Tls_set.tls_set).

I'm concerned, however, about directly copying code from paho. After all, asyncio-mqtt is meant to be a wrapper around paho and not a reimplementation of paho. Copying code goes against that. Furthermore, it increases the maintenance burden: We now have to support the copied code.

Ideally, we simply call the code from paho whenever we can. This way, if something gets fixed in paho.mqtt.Client.tls_set, said fix is automatically included in asyncio-mqtt. Let paho support the technical/protocol stuff and let us just support the asyncio stuff. :)

All that being said, I think you're on to something with your Tls_set.tls_set function (though I would probably just make it a free function and call it create_ssl_context). I think that your approach is the way it should have been implemented in paho from the start. Paho's authors chose another approach and, well, we just have to live with that.

For asyncio-mqtt I think that we should step in paho's footsteps (for better and worse) and simply call the existing tls_set function in Client.__init__. This is similar to what we already do for the username, will, and tls_context keyword arguments.

What do you think? Does it make sense?

from aiomqtt.

@Sohaib90, that's great to hear. Go for it. :) I'm ready to review the PR. 👍

from aiomqtt.

Okay great. I will add it and open a pull request soon then.

from aiomqtt.

Hi svinz, thanks for raising this issue. Let me have a look. :)

Indeed, the paho.mqtt.Client.tls_set functionality is not currently part of asyncio-mqtt. I'm definitely open for adding it. Preverably, as part of the asyncio_mqtt.Client.__init__'s keyword arguments. Do you want to add it via a pull request?

We do already have the tls_context keyword argument that you can use to set up TLS. Maybe that can work for you as well? :)

Let me know your thoughts.

from aiomqtt.

Yeah, I saw the tls_context in client.py.
After having a look at tls_set in paho.mqtt.client, it seems rather ok to implement this.

I made a small test by copy&paste from paho.mqtt.python.client.tls_set into asyncio_mqtt.Client.__init__ and it seems to work. I can try to make a PR tomorrow, though I'm not very proficient at this.

Alex

from aiomqtt.

Glad to hear that. :)

As far as I can see, Paho's Client.tls_set is just a convenience method that creates an ssl.SSLContext and then calls Client.tls_set_context. Seems fair enough to add this convenience to asyncio-mqtt as well. :)

Let me offer some design suggestions: Add this functionality via keyword arguments to the Client constructor. Maybe something like a tls_tuple keyword argument. E.g.:

from asyncio_mqtt import TlsTuple, Client

tls_tuple = TlsTuple(ca_certs=None, certfile=None, keyfile=None, cert_reqs=None, tls_version=None, ciphers=None)
client = Client("localhost", tls_tuple=tls_tuple)

Where TlsTuple is a new class that basically just structures the arguments together. You can use Python's collections.namedtuple to create it (ideally, we would use dataclasses.dataclass, but that is 3.7 only, and we want to support 3.6 for now).

In turn, the we can them simply forward the given tuple as follows (inside the __init__ function)

if tls_tuple is not None:
    self._client.tls_set(**tls_tuple._asdict())  # Assuming that tls_tuple is a namedtuple

How does that sound to you? Were you thinking of taking this in another direction? Let me know. :)

from aiomqtt.

I was more in to the not so elegant, but simple, solution, by using a @staticmethod and using the code from Paho Client.tls_set:

class Tls_set:
    @staticmethod
    def tls_set(ca_certs=None, certfile=None, keyfile=None, cert_reqs=None, tls_version=None, ciphers=None) -> ssl.SSLContext:

        if ssl is None:
            raise ValueError('This platform has no SSL/TLS.')

        if not hasattr(ssl, 'SSLContext'):
            # Require Python version that has SSL context support in standard library
            raise ValueError(
                'Python 2.7.9 and 3.2 are the minimum supported versions for TLS.')

        if ca_certs is None and not hasattr(ssl.SSLContext, 'load_default_certs'):
            raise ValueError('ca_certs must not be None.')

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

        # Configure context
        if certfile is not None:
            context.load_cert_chain(certfile, keyfile)

        if cert_reqs == ssl.CERT_NONE and hasattr(context, 'check_hostname'):
            context.check_hostname = False

        context.verify_mode = ssl.CERT_REQUIRED if cert_reqs is None else cert_reqs

        if ca_certs is not None:
            context.load_verify_locations(ca_certs)
        else:
            context.load_default_certs()

        if ciphers is not None:
            context.set_ciphers(ciphers)

        return context

and then use:

        context = Tls_set.tls_set(ca_certs="ca.pem")
        client = Client("test.mosquitto.org",tls_context=context,port=8883)

when setting up the client.
This way there is no changes to the Client-class either which is a good thing. Any thoughts?

from aiomqtt.

This would be super useful for me. So I'm giving it a bump 😄

from aiomqtt.

If this issue is still open, I would like to work on this issue and it via a PR request

from aiomqtt.