extmod/asyncio: Add ssl support with SSLContext. #11897

Carglglz · 2023-06-28T14:17:01Z

This adds asyncio ssl support and 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.

Follow-up to:

extmod/modssl_mbedtls: Add SSLContext methods. #13098
~~- [ ] extmod/modssl_mbedtls: Add cert time validation. #11896~~

This may close #8647, #9071, #8177, #5840, #7315, #9015

github-actions · 2023-06-28T14:28:39Z

Code size report:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64:  +616 +0.076% standard
      stm32:  +264 +0.067% PYBV10
     mimxrt:  +264 +0.073% TEENSY40
        rp2:  +272 +0.083% RPI_PICO
       samd:  +272 +0.104% ADAFRUIT_ITSYBITSY_M4_EXPRESS

codecov · 2023-06-29T01:03:33Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (f33dfb9) 98.39% compared to head (4848bc3) 98.40%.

❗ Current head 4848bc3 differs from pull request most recent head bfd6ad9. Consider uploading reports for the commit bfd6ad9 to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #11897   +/-   ##
=======================================
  Coverage   98.39%   98.40%           
=======================================
  Files         159      159           
  Lines       21055    21076   +21     
=======================================
+ Hits        20718    20739   +21     
  Misses        337      337

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Carglglz · 2023-07-13T17:24:12Z

Note that the "fix" is more a workaround than a real fix, I'm not sure where in the codebase the "read/write/close" events for stream ssl sockets are handled other than in modssl_mbedtls.c and why the bug affects the unix port but not the esp32 port...

dpgeorge · 2023-07-14T07:34:22Z

extmod/asyncio/stream.py

@@ -30,6 +36,15 @@ async def wait_closed(self):
    def read(self, n=-1):
        r = b""
        while True:
+            if hasattr(self.s, "ssl_pending"):


Is this ssl_pending only needed for the unix port to work correctly? If so I would rather fix unix by making it work correctly when polling an ssl socket.

I have not tested any other port besides esp32 and unix, but I think that is the case. It would be nice if someone could test with rp2/stm32 ports to confirm. Revelant test is tests/multi_net/asyncio_tls_server_client_readline.py where the unix port will get stuck without the fix due to this:

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.

I did try to fix it but to no avail. I only came to the conclusion that stream ssl sockets seem to behave different from plain stream sockets.

If so I would rather fix unix by making it work correctly when polling an ssl socket.

Yes that would be better and I'm curious to see where exactly is the offending line of code 👀

I can confirm that the tests/multi_net/asyncio_tls_server_client_readline.py test works with the ssl_pending code there and fails when it is removed.

But, by enabling MICROPY_PY_SELECT, it will work without the ssl_pending code.

So I will fix the unix port so that it can work without needing to enable that option.

BTW, to run this test I actually had to regenerate the certificate and key, because otherwise it gave a "certificate has expired" error.

But, by enabling MICROPY_PY_SELECT, it will work without the ssl_pending code.

well that seems like an easy fix 😅

BTW, to run this test I actually had to regenerate the certificate and key, because otherwise it gave a "certificate has expired" error.

I will update the tests too with a longer expiration date then 👍🏼

But, by enabling MICROPY_PY_SELECT, it will work without the ssl_pending code.
So I will fix the unix port so that it can work without needing to enable that option

I will wait for the fix then... 👀

FWIW I did try to solve it fixing the poll_set_all_are_fds logic checking the poll_set length but that turned out to be a slippery slope... 😕 so this was the only reliable method I could think of instead...

See #12224 for a fix.

I've just tested #12224 and it seems to be working as expected this time 👍🏼
I've pulled the changes here and CI tests should pass now again.

That PR is now merged to master.

Rebased and all CI tests passing, I guess this is finally ready!

dpgeorge · 2023-12-12T06:13:57Z

@Carglglz can you please rebase this on the latest master? Then it's hopefully ready to be merged.

Carglglz · 2023-12-12T18:18:10Z

@dpgeorge I've update the tests with the os.stat logic too 👍🏼

tests/multi_net/asyncio_tcp_tls_server_client.py

tests/multi_net/asyncio_tls_server_client.py

dpgeorge · 2023-12-13T00:06:43Z

tests/net_inet/asyncio_http_https_opc.py

+
+client_ctx = ssl.SSLContext(ssl.PROTOCOL_TLS_CLIENT)
+client_ctx.verify_mode = ssl.CERT_REQUIRED
+client_ctx.load_verify_locations(cafile=ca_cert_chain)


This test (and the other 3 added to the net_inet directory) don't run on CPython (at least not 3.11.6) because CPython's load_verify_locations() requires the file to be in PEM format, not DER.

The fix for that is to load the data from the DER here and then pass it in as cadata.

But, that still doesn't seem to have the correct behaviour because I would think that you don't need the line below (the call to load_default_certs()) because it should be enough to load just the DER certificate from mpycert.der in order to access micropython.org. But if I remove the load_default_certs() then CPython cannot connect to micropython.org. If I leave load_default_certs() in place then I can just remove this load_verify_locations() call and CPython works.

So it seems this test is not working as expected on CPython.

The fix for that is to load the data from the DER here and then pass it in as cadata.

I agree

But, that still doesn't seem to have the correct behaviour because I would think that you don't need the line below (the call to load_default_certs()) because it should be enough to load just the DER certificate from mpycert.der in order to access micropython.org. But if I remove the load_default_certs() then CPython cannot connect to micropython.org. If I leave load_default_certs() in place then I can just remove this load_verify_locations() call and CPython works.

I could reproduce this, and it seems mbedtls is a bit more "permissive" than OpenSSL when it comes to certificate validation, i.e. mbedtls is OK using "intermediate" CA certificates, while OpenSSL wants the "ultimate" root CA certificate.

The fix is using this root CA certificate that both MicroPython and CPython accept 👍🏼

FWIW I got this root CA certificate from micropython/micropython-lib#633 so if you have time you may want to have a look.

from README.md

5. Default ca-bundle.

certifi package will include CA Root certs ISRG Root X1 which is used to validate MicroPython mip server and DigiCert Global Root CA, which is used to validate github.com. This way mip can validate by default that the package being installed comes in fact from MicroPython package index server, or from the github repo of a 3rd party package.

OK, using the ISRG certificate is a good idea. Except it doesn't seem to work for me, running that test I get:

CPython:

ssl.SSLError: not enough data: cadata does not contain a certificate (_ssl.c:4020)

MicroPython:

ValueError: invalid cert

I was suspecting something may go wrong after this message, but I wasn't sure...

$ git add net_inet/isrg.der warning: in the working copy of 'tests/net_inet/isrg.der', CRLF will be replaced by LF the next time Git touches it

~~I guess I will need to add this in the test itself, the good thing is that it should be valid for a while (notAfter=Jun 4 11:04:38 2035 GMT)~~

OK: adding *.der binary to .gitattributes does fix it. 😅

tests/net_inet/asyncio_http_https_opc.py

tests/net_inet/asyncio_tls_open_connection.py

tests/multi_net/asyncio_tcp_tls_server_client.py.exp

tests/multi_net/asyncio_tls_tcp_server_client.py.exp

This adds asyncio ssl support with SSLContext and the corresponding tests in `tests/net_inet` and `tests/multi_net`. Note that not doing the handshake on connect will delegate the handshake to the following `mbedtls_ssl_read/write` calls. However if the handshake fails when a client certificate is required and not presented by the peer, it needs to be notified of this handshake error (otherwise it will hang until timeout if any). Finally at MicroPython side raise the proper mbedtls error code and message. Signed-off-by: Carlos Gil <carlosgilglez@gmail.com>

dpgeorge · 2023-12-14T01:37:11Z

Merged! Thanks again @Carglglz for sticking with this and getting it through.

Carglglz · 2023-12-14T02:30:12Z

@dpgeorge Thank you for the support and for helping to get this through! 👍🏼

Carglglz mentioned this pull request Jun 28, 2023

extmod/modssl: Add SSLContext class. #11862

Merged

Carglglz force-pushed the asyncio-tls branch from 248235a to a0a02da Compare June 29, 2023 00:47

Carglglz force-pushed the asyncio-tls branch 4 times, most recently from 3936b2e to b83af3b Compare July 4, 2023 22:27

Carglglz mentioned this pull request Jul 4, 2023

extmod/ssl: Add SSLContext #8968

Closed

Carglglz force-pushed the asyncio-tls branch 2 times, most recently from 2175158 to ab3a8eb Compare July 7, 2023 15:08

dpgeorge added the extmod Relates to extmod/ directory in source label Jul 11, 2023

Carglglz force-pushed the asyncio-tls branch from ab3a8eb to de64e69 Compare July 11, 2023 02:44

dpgeorge reviewed Jul 14, 2023

View reviewed changes

Carglglz force-pushed the asyncio-tls branch 4 times, most recently from a68b999 to 6ac75ad Compare July 16, 2023 00:55

Carglglz mentioned this pull request Jul 17, 2023

lib/mbedtls: Update to mbedtls-v3.5.1. #8988

Merged

dpgeorge mentioned this pull request Aug 1, 2023

Switch unix port to common select module, and add POSIX optimisations to that module #12138

Merged

Carglglz force-pushed the asyncio-tls branch from 6ac75ad to a7a1fae Compare August 7, 2023 15:32

dpgeorge mentioned this pull request Aug 9, 2023

extmod/modssl_mbedtls: Reject ioctls that are not supported. #12195

Merged

Carglglz force-pushed the asyncio-tls branch 7 times, most recently from 6833d43 to 8d8ea5a Compare August 14, 2023 02:47

Carglglz force-pushed the asyncio-tls branch 9 times, most recently from d283028 to b20f564 Compare December 8, 2023 17:05

Carglglz mentioned this pull request Dec 10, 2023

micropython/aiohttp: Add aiohttp package. micropython/micropython-lib#778

Merged

1 task

Carglglz force-pushed the asyncio-tls branch from b20f564 to 824bc21 Compare December 10, 2023 18:24

dpgeorge mentioned this pull request Dec 12, 2023

extmod/uasyncio: Add ssl support to start_server. #7315

Closed

Carglglz force-pushed the asyncio-tls branch 2 times, most recently from 53c7abe to 1cf31f6 Compare December 12, 2023 18:05

dpgeorge reviewed Dec 13, 2023

View reviewed changes

dpgeorge mentioned this pull request Dec 13, 2023

moduasyncio: Add SSL support #5840

Closed

Carglglz force-pushed the asyncio-tls branch from 1cf31f6 to 8e5201f Compare December 13, 2023 04:11

dpgeorge reviewed Dec 13, 2023

View reviewed changes

tests/multi_net/asyncio_tcp_tls_server_client.py.exp Outdated Show resolved Hide resolved

dpgeorge reviewed Dec 13, 2023

View reviewed changes

tests/multi_net/asyncio_tls_tcp_server_client.py.exp Outdated Show resolved Hide resolved

Carglglz force-pushed the asyncio-tls branch from 8e5201f to e3e838f Compare December 13, 2023 16:41

dpgeorge force-pushed the asyncio-tls branch 2 times, most recently from aa7a74d to 4848bc3 Compare December 14, 2023 00:57

dpgeorge force-pushed the asyncio-tls branch from 4848bc3 to bfd6ad9 Compare December 14, 2023 01:21

dpgeorge merged commit bfd6ad9 into micropython:master Dec 14, 2023

Carglglz deleted the asyncio-tls branch December 14, 2023 02:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

extmod/asyncio: Add ssl support with SSLContext. #11897

extmod/asyncio: Add ssl support with SSLContext. #11897

Carglglz commented Jun 28, 2023 •

edited

Loading

github-actions bot commented Jun 28, 2023 •

edited

Loading

codecov bot commented Jun 29, 2023 •

edited

Loading

Carglglz commented Jul 13, 2023

dpgeorge Jul 14, 2023

Carglglz Jul 14, 2023 •

edited

Loading

dpgeorge Jul 15, 2023

Carglglz Jul 15, 2023

Carglglz Jul 15, 2023 •

edited

Loading

Carglglz Aug 14, 2023

dpgeorge Aug 14, 2023

Carglglz Aug 14, 2023

dpgeorge Aug 14, 2023

Carglglz Aug 14, 2023

dpgeorge commented Dec 12, 2023

Carglglz commented Dec 12, 2023

dpgeorge Dec 13, 2023

Carglglz Dec 13, 2023 •

edited

Loading

dpgeorge Dec 13, 2023

Carglglz Dec 13, 2023 •

edited

Loading

dpgeorge commented Dec 14, 2023

Carglglz commented Dec 14, 2023

extmod/asyncio: Add ssl support with SSLContext. #11897

extmod/asyncio: Add ssl support with SSLContext. #11897

Conversation

Carglglz commented Jun 28, 2023 • edited Loading

github-actions bot commented Jun 28, 2023 • edited Loading

codecov bot commented Jun 29, 2023 • edited Loading

Codecov Report

Carglglz commented Jul 13, 2023

Choose a reason for hiding this comment

Carglglz Jul 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Carglglz Jul 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpgeorge commented Dec 12, 2023

Carglglz commented Dec 12, 2023

Choose a reason for hiding this comment

Carglglz Dec 13, 2023 • edited Loading

Choose a reason for hiding this comment

5. Default ca-bundle.

Choose a reason for hiding this comment

Carglglz Dec 13, 2023 • edited Loading

Choose a reason for hiding this comment

dpgeorge commented Dec 14, 2023

Carglglz commented Dec 14, 2023

Carglglz commented Jun 28, 2023 •

edited

Loading

github-actions bot commented Jun 28, 2023 •

edited

Loading

codecov bot commented Jun 29, 2023 •

edited

Loading

Carglglz Jul 14, 2023 •

edited

Loading

Carglglz Jul 15, 2023 •

edited

Loading

Carglglz Dec 13, 2023 •

edited

Loading

Carglglz Dec 13, 2023 •

edited

Loading