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

extmod/ssl: Add SSLContext #8968

Closed
wants to merge 20 commits into from

Conversation

Carglglz
Copy link
Contributor

@Carglglz Carglglz commented Jul 26, 2022

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():
    • set_ciphers()
    • get_ciphers()
    • load_verify_locations()
    • load_cert_chain()
    • wrap_socket()
    • check_hostname
    • verify_mode
    • protocol
    • load_default_certs()
    • reset()
  • _SSLSocket.cipher()
  • Certificate verification errors as 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 👀

@Carglglz Carglglz marked this pull request as draft July 26, 2022 21:16
@Carglglz Carglglz force-pushed the ssl-add-SSLContext branch 2 times, most recently from b73f406 to 5234e1f Compare July 28, 2022 18:12
@Carglglz
Copy link
Contributor Author

Carglglz commented Jul 28, 2022

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 multi_net/ssl_context_rsa.py test and
I tried to add MBEDTLS_VERSION but it seems it is only available in MBEDTLS 3.x, that is why I introduced # MICROPY_SSL_MBEDTLS_3 macro, and why some unix port builds fails.

MBEDTLS_VERSION

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'

But esp-idf 4.x uses MBEDTLS 2.x.

Tests passes in esp32 too 👍🏼 .

[EDIT]

I added a multi_net/ssl_context_rsa.py test and
I tried to add MBEDTLS_VERSION but it seems it is only available in MBEDTLS 3.x, that is why I introduced # MICROPY_SSL_MBEDTLS_3 macro, and why some unix port builds fails.

Ok this is odd, it turns out that I at some point checked out mbedtls develop branch and then even doing make submodules to switch back to 1bc2c9cb8 HEAD, build_info.h is still available, not sure how 😕, giving the wrong build version.
Anyway I've just fixed it, now works in mbedTLS 2.x and it may be OK leaving the macro for mbedTLS 3.x (for a future upgrade maybe? --> 3.0-migration-guide (see #8988 )

@Carglglz Carglglz force-pushed the ssl-add-SSLContext branch 7 times, most recently from f0f5546 to 9009d80 Compare August 2, 2022 20:59
@Carglglz
Copy link
Contributor Author

Carglglz commented Aug 2, 2022

I've just added SSLContext.reset() method and a http_server_ssl_context.py example.
This allows to "reuse" the SSLContext. I guess this is not CPython compliant but I'm not sure how to balance between memory allocation and context reusability.
Right now what context does (or I think it does) is:

  1. Context initiation --> memory allocation --> load cert/key/cadata/conf
  2. Context wrap socket --> creates a ssl socket that use the allocated memory from context
  3. SSLSocket is closed and frees the allocated memory

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

@Carglglz
Copy link
Contributor Author

Carglglz commented Aug 22, 2022

Update on date/time certificate validation:

Ports using UNIX EPOCH

It turns out that enabling date/time certificate validation only requires defining these macros MBEDTLS_HAVE_TIME and MBEDTLS_HAVE_TIME_DATE in mbedtls_config.h, then mbedtls will use time() from time.h and expects to get current time in seconds since UNIX EPOCH (1970). So in platforms using UNIX EPOCH, enabling these macros will make it work.

Ports not using UNIX EPOCH

However 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 MBEDTLS_X509_BADCERT_FUTURE (certificate validity starts in the future)

Fix

A way to fix this is defining MBEDTLS_PLATFORM_TIME_ALT macro in mbedtls_config.h which exposes a function in mbedtls, mbedtls_platform_set_time that allows to set the function that mbedtls will use instead of time().
So I added this patch in commit 1eac341 which needs:
MICROPY_MBEDTLS_PLATFORM_TIME_ALT macro in mpconfigport.h and
MBEDTLS_PLATFORM_TIME_ALT macro in mbedtls_config.h.

Fix for ports not using time()

Enable macro MBEDTLS_PLATFORM_TIME_MACRO and defining a function in mbedtls_port.c to use RTC as in rp2 port.

Fix for ports not using time() and no time.h

See #9089

Problem with esp-idf and esp32 port:

Components option are configured using skdconfig.base, however till (approx) current esp-idf/master option CONFIG_MBEDTLS_PLATFORM_TIME_ALT does not exist, so MBEDTLS_PLATFORM_TIME_ALT must be manually added to esp-idf/components/mbedtls/mbedtls/include/mbedtls/config.h, after that it works on esp32 port too.

See time-related-functions-mbedtls

@Carglglz Carglglz force-pushed the ssl-add-SSLContext branch 2 times, most recently from 0af793d to fc4b81f Compare August 22, 2022 17:36
@Carglglz
Copy link
Contributor Author

Note about ussl.wrap_socket deprecation:

I "deprecated" ussl.wrap_socket (just commented out for now) in favour of ssl.wrap_socket that is backwards compatible with ussl.wrap_socket and with the docs api description (so it accepts kargs key, cert, or keyfile, certfile)

Signed-off-by: Carlos Gil <carlosgilglez@gmail.com>
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>
mbedtls_ssl_config conf;
mbedtls_x509_crt cacert;
mbedtls_x509_crt cert;
mbedtls_pk_context pkey;
Copy link
Member

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 *
Copy link
Member

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:
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 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:
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 _reload_ctx logic can be removed if the mbedtls state is properly split between SSLContext and SSLSocket (as discussed above).

@dpgeorge
Copy link
Member

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:

  • reworking the existing ssl module to introduce ssl.SSLContext
  • adding a lot of new methods and functionality to SSLContext
  • adding time support to certificates
  • adding support for SSL connections in asyncio
  • adding tests and examples

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.

@dpgeorge
Copy link
Member

See #11862 for an alternative implementation of just ssl.SSLContext (that's not intending to replace this whole PR).

@Carglglz
Copy link
Contributor Author

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.

Yes I know 😅 , that's exactly what I proposed here 👍🏼

@dpgeorge I think this is ready for review, I could squash some commits or split this PR if it is too big, whatever makes it easier.

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:

PR0: #11862
PR1: adding a lot of new methods and functionality to SSLContext
PR2: adding time support to certificates
PR3: adding support for SSL connections in asyncio
PR4: adding tests and examples

@dpgeorge
Copy link
Member

I will review the changes you proposed, but in general I'm OK as long as functionality is kept.

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.

@Carglglz
Copy link
Contributor Author

Carglglz commented Jul 4, 2023

Closing this in favour of #11862 #11888 #11896 #11897

@Carglglz Carglglz closed this Jul 4, 2023
Carglglz added a commit to Carglglz/micropython-lib that referenced this pull request Sep 1, 2023
	Add async mqtt simple client compatible with
	SSLContext from micropython/micropython#8968

Signed-off-by: Carlos Gil <carlosgilglez@gmail.com>
Carglglz added a commit to Carglglz/micropython-lib that referenced this pull request Sep 1, 2023
Add async mqtt simple client compatible with
SSLContext from micropython/micropython#8968

Signed-off-by: Carlos Gil <carlosgilglez@gmail.com>
Carglglz added a commit to Carglglz/micropython-lib that referenced this pull request Dec 13, 2023
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>
Carglglz added a commit to Carglglz/micropython-lib that referenced this pull request Dec 13, 2023
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>
@Carglglz Carglglz deleted the ssl-add-SSLContext branch January 21, 2024 17:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants