-
-
Notifications
You must be signed in to change notification settings - Fork 8.2k
extmod/ssl: Add SSLContext #8968
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
b73f406
to
5234e1f
Compare
Well I'm not sure if this is the right way to implement this, but it is a start. (C code probably needs some cleaning/fixing 🤷🏼♂️) I added a
MicroPython v1.19.1-224-g71cf755fc on 2022-07-29; darwin [GCC 4.2.1] version
Use Ctrl-D to exit, Ctrl-E for paste mode
>>> import ssl
>>> ssl.MBEDTLS_VERSION
'mbed TLS 2.16.10'
Tests passes in [EDIT]
Ok this is odd, it turns out that I at some point checked out |
f0f5546
to
9009d80
Compare
I've just added
So to reuse the context, it needs to be reinitiated to allocate memory again, or another option would be to stop the SSLSocket from freeing the allocated memory (but I'm not sure this is the way to go... 🤔 ) |
bd4f703
to
fef32b6
Compare
Update on date/time certificate validation: Ports using UNIX EPOCHIt turns out that enabling date/time certificate validation only requires defining these macros Ports not using UNIX EPOCHHowever there are some ports that use EPOCH 1/1/2000, which makes mbedtls "believe" that time is now - 30 years, failing to validate certs with FixA way to fix this is defining Fix for ports not using
|
0af793d
to
fc4b81f
Compare
Note about I "deprecated" |
8aaff88
to
99bbae2
Compare
Signed-off-by: Carlos Gil <carlosgilglez@gmail.com>
Signed-off-by: Carlos Gil <carlosgilglez@gmail.com>
This enables MBEDTLS_PLATFORM_TIME_ALT which is needed for ssl certs datetime validation. This is due to esp32 using EPOCH 1/1/2000 to get current time in seconds which is not what mbedlts expects. MBEDTLS_PLATFORM_TIME_ALT gives the option to define an alternative function to get current time. Signed-off-by: Carlos Gil <carlosgilglez@gmail.com>
Signed-off-by: Carlosgg <carlosgilglez@gmail.com>
Make ssl_poll.py use ssl module and add ssl.py module to unix coverage manifest.py Signed-off-by: Carlos Gil <carlosgilglez@gmail.com>
Signed-off-by: Carlos Gil <carlosgilglez@gmail.com>
Signed-off-by: Carlos Gil <carlosgilglez@gmail.com>
Signed-off-by: Carlos Gil <carlosgilglez@gmail.com>
Signed-off-by: Carlos Gil <carlosgilglez@gmail.com>
Allow reuse context after handshake failure. Signed-off-by: Carlos Gil <carlosgilglez@gmail.com>
This fix async stream read methods in unix port. The reason is stream ssl sockets seem to behave different from plain stream sockets.The main difference is doing partial reads on ssl sockets will clear any write/read/close "event" flag even if there is still pending data to be read. So registering the ssl socket again in the poller will wait there until a new event or timeout. The fix is check first if the ssl socket has pending data and read/return it instead of registering in the poller to wait for an event. Signed-off-by: Carlos Gil <carlosgilglez@gmail.com>
Reading from a stream ssl socket too soon will return `None` so this avoids adding the bytes buffer with None raising `TypeError`. Signed-off-by: Carlos Gil <carlosgilglez@gmail.com>
Signed-off-by: Carlos Gil <carlosgilglez@gmail.com>
Signed-off-by: Carlos Gil <carlosgilglez@gmail.com>
d78335a
to
3d071ff
Compare
mbedtls_ssl_config conf; | ||
mbedtls_x509_crt cacert; | ||
mbedtls_x509_crt cert; | ||
mbedtls_pk_context pkey; |
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.
All this context/state has been added here (in the SSLContext object) but it also remains in the SSLSocket object. I don't think that's the correct approach because (1) it uses additional memory, to duplicate the state in each SSLSocket, (2) it means that a multiple SSLSockets can't share state from the same SSLContext.
I think the only thing that should be in the SSLSocket struct is mbedtls_ssl_context
(the naming is poor, but mbedtls_ssl_config
maps to SSLContext and mbedtls_ssl_context
maps to SSLSocket).
@@ -0,0 +1 @@ | |||
from .ssl import * |
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 file is not doing anything, it's not in the manifest (and isn't really needed).
) | ||
|
||
|
||
class SSLContext: |
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 should inherit from the native "ssl context" class, and add methods that are missing, rather than have a ctx
member.
raise ValueError("check_hostname requires server_hostname") | ||
# to be substituted by e.g. _ussl._context_wrap_socket in modussl_mbedtls.c ?: | ||
# _ussl._context_wrap_socket(*args, **kargs) | ||
if self._reload_ctx: |
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 _reload_ctx
logic can be removed if the mbedtls state is properly split between SSLContext and SSLSocket (as discussed above).
Thanks @Carglglz for working on this and keeping it up to date (rebased). In addition to the above comments, another issue with this PR is that it's doing too much. From what I can tell it is:
Don't get me wrong, these are all good features to have. But it's hard to review when there is so much going on. And in this case a lot of the components are independent enough that they could be in separate PRs. |
See #11862 for an alternative implementation of just |
Yes I know 😅 , that's exactly what I proposed here 👍🏼
see full comment in #8968 (comment) I will review the changes you proposed, but in general I'm OK as long as functionality is kept. Probably the best approach is to rebase this PR on #11862 and split it into smaller PRs based on this:
|
OK, thanks for being flexible with how we implement this. Since this SSL stuff is fundamental to a lot of other components that will be built on top of it, we need to make sure it's done well from the beginning. If you could review #11862 that would be great. |
Add async mqtt simple client compatible with SSLContext from micropython/micropython#8968 Signed-off-by: Carlos Gil <carlosgilglez@gmail.com>
Add async mqtt simple client compatible with SSLContext from micropython/micropython#8968 Signed-off-by: Carlos Gil <carlosgilglez@gmail.com>
This adds a method to include default/installable certificates from Mozilla's CA store which could solve the default certs problem from micropython/micropython#8968 Signed-off-by: Carlos Gil <carlosgilglez@gmail.com>
This adds a method to include default/installable certificates from Mozilla's CA store which could solve the default certs problem from micropython/micropython#8968 Signed-off-by: Carlos Gil <carlosgilglez@gmail.com>
This is a proof of concept/demo of what was proposed in #8915 and #8252 to get closer to CPython
ssl.py
module:This implements a very basic version of
SSLContext
in MicroPython and adds new features in C:ssl.SSLContext()
:_SSLSocket.cipher()
ValueError:
+ string indicating failure reasons.I also made a demo test
tests/net_inet/test_ssl_context_client.py
that works in CPython, unix and esp32 port.There is still some questions that need to be resolved (see comments in
extmod/ssl/ssl.py
), but I think this is good enough to get the ball rolling 👀