Skip to content

moduasyncio: Add SSL support #5840

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

Closed
wants to merge 1 commit into from
Closed

Conversation

tve
Copy link
Contributor

@tve tve commented Mar 29, 2020

This PR sits on top of #5819 and #5825#6615 and adds SSL support to uasyncio. Update: I backed out #5819 and #5825#6615 to make the PR easier to review on github, obviously CI now fails. I'll do the proper rebase once the fate of these other PRs is determined.

Adding the wrap_socket to Stream.open_connection is relatively straight-forward. It's different from CPython (no SSLContext) but compatible. The trickier part is to deal with poll, see comments in modussl_mbedtls. I believe I came up with a clean solution but so far I have failed to produce a test case that generates the problem so I can have a fail-before/success-after confirmation that the solution actually works or actually is necessary. I need to think about a test case more but if anyone can construct one I'd love that too!

I added a errstr class function to ussl in order to convert an ssl error number to a string. This is needed when using uasyncio because it uses read/write which means ussl doesn't raise an OSError (which would have the error string) and when the app gets the OSError through the Stream stuff it is again unintelligible. Now at least one can write something like if e.args[0] < -120: print(ssl.errstr(e.args[0])).

Open to-do items:

  • figure out whether the poll patch is needed and works
  • add strerr and poll patch to axtls
  • make recv and send run their OSError raising through ussl_raise_error to get an error message in there
  • add errstr to docs
  • add wrap_socket to Server.serve
  • test against esp32 espidf v3
  • test on other platforms

I should say that the tests don't all pass. Specifically, net_inet/test_tls_sites fails on the esp32. I'm pretty sure the issue is an out-of-memory in esp-idf situation. It ends up dropping wifi. I believe if I fix #5835 there may be just enough memory again to sail through...

I'll proceed once I have feedback

tve added a commit to tve/micropython that referenced this pull request Mar 31, 2020
@tve tve force-pushed the uasyncio-ssl2 branch from 4830774 to 837268c Compare April 2, 2020 17:59
@tve
Copy link
Contributor Author

tve commented Apr 2, 2020

Force-push to back out #5819 and #5825.

@peterhinch
Copy link
Contributor

This sounds great - problems with SSL/TLS constantly crop up on the forum. I've only succeeded with it on the Pyboard D.

Will it work with

  • Nonblocking sockets?
  • Ports other than ESP32?

I was under the impression that some ports don't use mbedtls.

@tve
Copy link
Contributor Author

tve commented Apr 19, 2020

Will it work with

  • Nonblocking sockets?

Yes, that's a big motivation behind this since the new uasyncio uses non-blocking sockets. This is why all this requires #5825

  • Ports other than ESP32?

The actual mbedtls library is different for the esp32 vs other ports because the esp-idf version is used for the esp32 instead of the one in the MP tree. I now have a PYBD so I'll test there as well.

I was under the impression that some ports don't use mbedtls.

Correct, unix and esp8266 use axtls. I am testing the unix port. I don't know what to do about the esp8266 port, I don't want to spend all my time trying to craft test cases that don't run out of memory...

Note that this PR can't merge until #5850, #5819 and #5825 are in...

@pumelo
Copy link

pumelo commented Aug 18, 2020

👍
@tve thank you very much for the patch about poll stuff in extmod/modussl_mbedtls.c. I had an issue where I only received the first byte of the mqtt message and only after sending the ping I received the rest of the mqtt message! This patch solved the issue and the complete message now arrives without writing a ping!

💐

@dpgeorge dpgeorge added the extmod Relates to extmod/ directory in source label Sep 14, 2020
@dpgeorge
Copy link
Member

The trickier part is to deal with poll, see comments in modussl_mbedtls. I believe I came up with a clean solution

Yes, I like your approach, it's very clean indeed. It effectively allows you to poll the SSL object itself, and then it takes care of polling the underlying socket to get what you want (eg it may poll for writes on the socket even if you poll the SSL only for reads). That seems like a good way to abstract/separate things. (In contrast to the use of SSLWantReadError and SSLWantWriteError in CPython which are telling you what the SSL object expects from the socket, and really exposing the user of the SSL object to the implementation details of the SSL protocol and how it utilises the socket to read/write raw data.)

A few points about this approach:

  1. I'm guessing it won't work efficiently on *nix/windows (and maybe RTOS' like Zephyr), because you won't be able to register the SSL object with select/poll (which can only take file descriptors). So on these platforms select/poll needs to be a busy loop checking the SSL object (maybe?). It could be that uevent helps here.
  2. I haven't run any tests, but it may be that this clean approach is actually very inefficient: eg if you want to write some data then you poll for POLLOUT on the SSL object. It returns immediately because there is space in the underlying TCP socket. But writes cannot progress due to a WANT_READ indication from mbedtls. So the write returns None (non-blocking return) and you go round in the loop waiting to POLLOUT again, which returns immediately again, etc. Really what needs to happen is that it does POLLIN and waits for new data before progressing.

I added a errstr class function to ussl in order to convert an ssl error number to a string. This is needed when using uasyncio because it uses read/write which means ussl doesn't raise an OSError

I think there's a clean solution here: you can actually raise an OSError directly in the socket_read/socket_write stream-protocol-C-methods, instead of setting *errcode = ret and returning MP_STREAM_ERROR. So if the error is one from deep within mbedtls (errno < -120) then raise directly, otherwise return MP_STREAM_ERROR.

figure out whether the poll patch is needed and works

I was also trying to write a test case that shows this is needed, but didn't succeed yet.

add strerr and poll patch to axtls

I'm not sure if it's possible at all to get this working with axtls.... so might need to drop that and test with mbedtls on unix instead (unix will already build with mbedtls).

tve added a commit to tve/micropython that referenced this pull request Dec 24, 2020
vstr_init_len(&vstr, 80);

// Including mbedtls_strerror takes about 16KB on the esp32 due to all the strings
#if 1
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note to self: this needs adaptation per tve@cdb0143

@tve
Copy link
Contributor Author

tve commented Feb 9, 2021

Mentioned in #6865. Still blocked behind #5825#6615.

@dpgeorge
Copy link
Member

I think all prerequisites for this are now merged. Can you please rebase on master?

@peterhinch
Copy link
Contributor

What was the outcome re ESP8266?

@tve tve force-pushed the uasyncio-ssl2 branch 3 times, most recently from b4835ee to de583a7 Compare March 1, 2021 06:33
@tve tve marked this pull request as draft March 1, 2021 06:54
@tve tve force-pushed the uasyncio-ssl2 branch from 94b94c7 to 611d30a Compare March 1, 2021 06:55
@chrisovergaauw
Copy link
Contributor

Any idea when this will be merged?

tannewt pushed a commit to tannewt/circuitpython that referenced this pull request Jan 13, 2022
@dpgeorge
Copy link
Member

The underlying non-blocking functionality in modussl_mbedtls.c has been implemented in ed58d6e

@tve if you're interested to keep working on this PR for the uasyncio SSL support (which we definitely want!) then please rebase on latest master to pick up the mbedtls changes. Effectively you won't need to have any changes to C code in this PR anymore.

@Carglglz
Copy link
Contributor

@dpgeorge I added uasyncio support in #8968 based on this PR, since CPython uses SSLContext in both asyncio.open_connection and asyncio.start_server. So let me know if you want to implement it.

@tve
Copy link
Contributor Author

tve commented Dec 19, 2022

I'm swamped at the moment, so I won't be able to get to this in the coming days, sorry...

@dpgeorge
Copy link
Member

Closing in favour of #11897.

@dpgeorge dpgeorge closed this Dec 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
extmod Relates to extmod/ directory in source
Projects
None yet
Development

Successfully merging this pull request may close these issues.

extmod/modussl_mbedtls: change getpeercert for a callback
6 participants