Skip to content

ssl: restructure micropython interface in a tls module #793

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

Merged
merged 2 commits into from
Feb 7, 2024

Conversation

felixdoerre
Copy link
Contributor

This changes correspond to micropython/12259.

@@ -90,7 +90,7 @@ def request(
try:
s.connect(ai[-1])
if proto == "https:":
s = ussl.wrap_socket(s, server_hostname=host)
s = tls.SSLContext(tls.PROTOCOL_TLS_CLIENT).wrap_socket(s, server_hostname=host)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Testing this change, it no longer works because verify_mode defaults to CERT_REQUIRED... I guess that's secure, but we really need to make this work like it did before. So this needs to do ctx.verify_mode = CERT_NONE before wrapping the socket.

Similarly with the umqtt and urllib changes in this PR.

Copy link
Contributor Author

@felixdoerre felixdoerre Feb 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll change the behavior for requests and urllib, as we don't ship a CA list by default we can't enable ssl verification without the user choosing it. (Previously, using the ssl library, with the legacy ssl.wrap_socket did disable verification). For umqtt, I've adjusted the constructor to not take a boolean parameter for ssl anymore, but an sslcontext instead. So if someone wants to enable insecure ssl, they need to disable verification in the ssl context themselves (or load ca certificates or install a cert callback).

Ideally we should have a way to do that for urllib and requests as well. Looking at the documentation of their python counterpart, urllib has a capath parameter, and requests has a verify parameter, to point to a ca file. I'm not sure how strictly to cpython subset we adhere here, but I would like to implement parameters to allow passing a ca certificate (by bytes object and not necessarily as file) for both modules.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, yes, umqtt now passes in the SSLContext, so no need to change the verify_mode for that.

I'm not sure how strictly to cpython subset we adhere here, but I would like to implement parameters to allow passing a ca certificate for both modules.

Yes, that would be a good addition, allowing to pass in certs. Ideally it would follow the CPython way. Of course that's for a separate PR!

@dpgeorge
Copy link
Member

dpgeorge commented Feb 6, 2024

Please can you change the module() call in ssl/manifest.py to:

module("ssl.py", opt=3)

That well help keep the code size down.

There don't seem to be any MQTT implementations that expect an empty
username (instead of the field missing), so the check for unused `user` can
be simplified.

Signed-off-by: Felix Dörre <felix@dogcraft.de>
MicroPython now supplies SSL/TLS functionality in a new built-in `tls`
module.  The `ssl` module is now implemented purely in Python, in this
repository.  Other libraries are updated to work with this scheme.

Signed-off-by: Felix Dörre <felix@dogcraft.de>
@dpgeorge dpgeorge merged commit 35d41db into micropython:master Feb 7, 2024
@dpgeorge
Copy link
Member

dpgeorge commented Feb 7, 2024

Thanks for this, now merged.

@@ -16,8 +16,7 @@ def __init__(
user=None,
password=None,
keepalive=0,
ssl=False,
ssl_params={},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems now like a "breaking" change, one needs to create and pass a SSLContext, see e.g. https://github.com/orgs/micropython/discussions/13624, I think this should be something like

if self.ssl:
             if hasattr(self.ssl, "wrap_socket"):
                 self.sock = self.ssl.wrap_socket(self.sock, **self.ssl_params)
             else:
                 import ssl as _ssl

                 self.sock = _ssl.wrap_socket(self.sock, **self.ssl_params)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I was not aware that breaking changes here are not possible. I would argue that this breaking change is good here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this breaking change is a fair change to make. We need to improve and make progress on things and sometimes it's not practical to retain backwards compatibility on everything.

In this case using the new library with old user code will raise an exception if the ssl argument is used. So it's easy for the user to know that things need to be changed.

And the new scheme matches better how SSL works in asyncio, passing in an SSLContext.

In the end a user can just keep using an old version of umqtt if needed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then document this somewhere in the umqtt's README or next release info should be fine 👍🏼

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants