Skip to content

extmod/modussl: fix socket and ussl read/recv/send/write errors for non-blocking sockets #5825

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 2 commits into from

Conversation

tve
Copy link
Contributor

@tve tve commented Mar 28, 2020

This PR attempts to make read/recv/send/write behave identically on non-blocking sockets and on SSL wrappers with a non-blocking socket underneath. The goal is for stuff like EAGAIN and EINPROGRESS or None returns to be correct and the same. In the end, that makes it easier to write software that can use plain or SSL sockets.

The PR includes a test that checks all combinations for connect+op on non-blocking sockets with and without ssl. In order to get there, this PR also implements send/recv on SSL "sockets", that makes for one less exception (the code added is minimal, so it seemed worth eliminating that source of frustration). Funny enough, CPython doesn't have read/write on sockets, gaaaa! I've tested on esp32 and unix so far, I hope to try esp8266 as well. I don't have any of the other platforms. The test passes on esp32 and fails on unix, the failure is because AXTLS can't quite do non-blocking SSL handshakes.

This PR includes #5819 'cause I can't get this one right without the error messages produced by the other one. I backed-out #5819 so the "Files changed" in github is more reasonable and it's easier to review. The downside is that CI will fail due to missing functions. I'll rebase on master once the fate of #5819 is decided.

I'm planning to add a test that performs a full non-blocking SSL connection, but I wanted to get the PR started. done

@tve tve changed the title extmod/modussl_mbedtls: fix socket and mbedssl read/recv/send/write errors for non-blocking sockets extmod/modussl: fix socket and ussl read/recv/send/write errors for non-blocking sockets Mar 29, 2020
@tve
Copy link
Contributor Author

tve commented Mar 29, 2020

I added a test performing complete non-blocking connections. Unfortunately run-tests doesn't like it and reports a crash even though pyboard runs it just fine:

> ./run-tests --target esp32 --device /dev/ttyUSB1 net_inet/test_tls_nonblo
ck.py
FAIL  net_inet/test_tls_nonblock.py
1 tests performed (34 individual testcases)
0 tests passed
1 tests failed: test_tls_nonblock
> cat net_inet_test_tls_nonblock.py.out
CRASH⏎
> ../tools/pyboard.py --device /dev/ttyUSB1 net_inet/test_tls_nonblock.py
('172.217.11.78', 443)
wrapped
wrote
read
google.com ok
...
('151.101.128.223', 443)
wrapped
wrote
read
pypi.org ok
('149.154.167.220', 443)
wrapped
api.telegram.org -103
('172.217.11.83', 443)
api.pushbullet.com [Errno 113] EHOSTUNREACH
DONE

tve added a commit to tve/micropython that referenced this pull request Mar 31, 2020
@tve tve force-pushed the modussl-nonblocking branch from eafe31a to 016c883 Compare April 2, 2020 06:21
@tve
Copy link
Contributor Author

tve commented Apr 2, 2020

CI for this PR fails. On stm32 mbedtls doesn't include mbedtls_strerr, I can either disable the string error generation or enable that function. Need guidance. I'm trying a #ifdef that checks what mbedtls includes.
On MacOS the ussl_basic test fails. I don't own a mac and run-tests says nothing other than that the test fails. Need help with that...
Overall, I'm happy to fix stuff here, but need some general thumbs-up/down first.

@tve tve force-pushed the modussl-nonblocking branch from ce7a548 to f83c09e Compare April 2, 2020 17:20
@tve
Copy link
Contributor Author

tve commented Apr 2, 2020

I backed-out #5819 so the "Files changed" in github is more reasonable and it's easier to review. The downside is that CI will fail due to missing functions. I'll rebase on master once the fate of #5819 is decided.

@tve tve force-pushed the modussl-nonblocking branch from f83c09e to f08a6b9 Compare April 20, 2020 21:49
@dpgeorge dpgeorge added the extmod Relates to extmod/ directory in source label Sep 14, 2020
@dpgeorge
Copy link
Member

@tve could you please rebase this on latest master (mainly to pick up the improvements to mbedtls error strings)?

The PR includes a test that checks all combinations for connect+op on non-blocking sockets with and without ssl. In order to get there, this PR also implements send/recv on SSL "sockets", that makes for one less exception (the code added is minimal, so it seemed worth eliminating that source of frustration).

I'm not sure about adding send/recv: in CPython there's the concept of SSLObject and SSLSocket. The former is the lower-level entity with just read/write. The latter is higher-level and intends to be a swap-in replacement for a socket.socket, so has send/recv as well as all the other socket methods, accept, sendall, etc. IMO MicroPython only needs to implement SSLObject, with just read/write. I don't see any good use for send/recv (and if we add those, the scope would then be expanded to add accept/sendall/etc).

Funny enough, CPython doesn't have read/write on sockets, gaaaa!

You need to do socket.makefile("rwb") to get an entity with read/write in CPython (and MicroPython also provides makefile() to write compatible code).

@tve
Copy link
Contributor Author

tve commented Sep 15, 2020

I'll try to get the rebase started today.

I knew you would have issues with send/recv. I did not realize the SSLObject vs SSLSocket difference. For me the primary reason to implement send/recv is to be able to write tests that run the same on all platforms 'cause I could not keep all the combos of ssl/non-ssl blocking/non-blocking read/write/recv/send straight otherwise. Even as-is the axtls platforms are slightly different.

@dpgeorge
Copy link
Member

For or me the primary reason to implement send/recv is to be able to write tests that run the same on all platforms

Is it not possible to do this using just read/write? What are the issues there?

@tve tve force-pushed the modussl-nonblocking branch from f08a6b9 to 40a4b51 Compare September 17, 2020 17:55
@tve
Copy link
Contributor Author

tve commented Sep 17, 2020

I did a rebase, it would be really helpful to get #5968 in first so I can fix the tests only once. It ended up with a question about what you want...

WRT send/recv/read/write:
CPython socket has send/recv, no read/write. MP socket has send/recv/read/write, so ensuring that MP non-blocking sockets work the same as for CPython is not really possible using a straight-forward test since CPython is missing read/write, so to ensure that those produce the same errors for blocking/non-blocking they have to be tested against canned expectations.
The same is true for sockets wrapped with ssl.wrap_socket: CPython has send/recv, no read/write. You're saying MP should have read/write no send/recv? Or maybe you're saying that while we use wrap_socket in MP we shouldn't use it in CP when writing tests?
In uasyncio/stream the implementation uses the socket's read/write, so that works with sockets on MP because they implement read/write even though CPython doesn't. Same issue when enabling support for SSL there.

Here's what I see in CPython 3.8.5:

>>> s=socket.socket()
>>> s.connect(socket.getaddrinfo("www.google.com", 443)[0][-1])
>>> ssl.wrap_socket(s)
<ssl.SSLSocket fd=3, family=AddressFamily.AF_INET, type=SocketKind.SOCK_STREAM, proto=0, laddr=('192.168.0.2', 53644), raddr=('142.250.68.100', 443)>
>>> s.
s.accept(           s.getblocking(      s.recvfrom(         s.set_inheritable(
s.bind(             s.getpeername(      s.recvfrom_into(    s.setblocking(
s.close(            s.getsockname(      s.recvmsg(          s.setsockopt(
s.connect(          s.getsockopt(       s.recvmsg_into(     s.settimeout(
s.connect_ex(       s.gettimeout(       s.send(             s.shutdown(
s.detach(           s.listen(           s.sendall(          s.timeout
s.dup(              s.makefile(         s.sendfile(         s.type
s.family            s.proto             s.sendmsg(
s.fileno(           s.recv(             s.sendmsg_afalg(
s.get_inheritable(  s.recv_into(        s.sendto(

I was being driven nuts by all these inconsistencies and ended up ensuring TLS supports read/write/send/recv like regular sockets on MP, that was quite the relief. It's now easy to go back and just delete, but someone has to actually dig into all this and decide what to delete, i.e., how compatible you want to be with CPython and what the total fall-out is... Or maybe I'm totally confused and I'm looking at he wrong stuff altogether? (Quite likely...)

@dpgeorge
Copy link
Member

I did a rebase, it would be really helpful to get #5968 in first so I can fix the tests only once. It ended up with a question about what you want...

Ok, will check that.

Here's what I see in CPython 3.8.5:

It seems your REPL code is wrong: you tab-complete on the normal socket, not the ssl wrapped object! On my CPython 3.8.5 the ssl wrapped object has read/write.

You can use s.makefile("rwb") on a normal socket to add read/write methods (also works on MicroPython so you can write portable code).

@tve
Copy link
Contributor Author

tve commented Sep 18, 2020

Ugh, my mind clearly was elsewhere...
Retry:

~> python
Python 3.8.5 (default, Jul 27 2020, 08:42:51)
[GCC 10.1.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import socket, ssl
>>> s=socket.socket()
>>> s.connect(socket.getaddrinfo("www.google.com", 443)[0][-1])
>>> s=ssl.wrap_socket(s)
>>> s.
s.accept(                        s.getsockopt(                    s.sendto(
s.bind(                          s.gettimeout(                    s.server_hostname
s.cipher(                        s.listen(                        s.server_side
s.close(                         s.makefile(                      s.session
s.compression(                   s.pending(                       s.session_reused
s.connect(                       s.proto                          s.set_inheritable(
s.connect_ex(                    s.read(                          s.setblocking(
s.context                        s.recv(                          s.setsockopt(
s.detach(                        s.recv_into(                     s.settimeout(
s.do_handshake(                  s.recvfrom(                      s.shared_ciphers(
s.do_handshake_on_connect        s.recvfrom_into(                 s.shutdown(
s.dup(                           s.recvmsg(                       s.suppress_ragged_eofs
s.family                         s.recvmsg_into(                  s.timeout
s.fileno(                        s.selected_alpn_protocol(        s.type
s.get_channel_binding(           s.selected_npn_protocol(         s.unwrap(
s.get_inheritable(               s.send(                          s.verify_client_post_handshake(
s.getblocking(                   s.sendall(                       s.version(
s.getpeercert(                   s.sendfile(                      s.write(
s.getpeername(                   s.sendmsg(
s.getsockname(                   s.sendmsg_afalg(

So the CPython SSL socket has read/write/send/recv.

@tve
Copy link
Contributor Author

tve commented Dec 24, 2020

superseded by #6615

@tve tve closed this Dec 24, 2020
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.

3 participants