-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Conversation
6f45bd9
to
dada525
Compare
dada525
to
4422566
Compare
@@ -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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
Please can you change the module("ssl.py", opt=3) That well help keep the code size down. |
4422566
to
4ec6a3a
Compare
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>
4ec6a3a
to
35d41db
Compare
Thanks for this, now merged. |
@@ -16,8 +16,7 @@ def __init__( | |||
user=None, | |||
password=None, | |||
keepalive=0, | |||
ssl=False, | |||
ssl_params={}, |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 👍🏼
This changes correspond to micropython/12259.