extmod/modussl*: Initial support for non-blocking mode. #4481

pfalcon · 2019-02-08T10:01:41Z

For this, add wrap_socket(do_handshake=False) param. CPython doesn't have
such a param at a module's global function, and at SSLContext.wrap_socket()
it has do_handshake_on_connect param, but that uselessly long.

Beyond that, make write() handle not just MBEDTLS_ERR_SSL_WANT_WRITE, but
also MBEDTLS_ERR_SSL_WANT_READ, as during handshake, write call may be
actually preempted by need to read next handshake message from peer.
Likewise, for read(). And even after the initial negotiation, situations
like that may happen e.g. with renegotiation. Both MBEDTLS_ERR_SSL_WANT_READ
and MBEDTLS_ERR_SSL_WANT_WRITE are however mapped to the same None return
code. The idea is that if the same read()/write() method is called repeatedly,
the progress will be made step by step anyway. The caveat is if user wants
to add the underlying socket to uselect.poll(). To be reliable, in this case,
the socket should be polled for both POLL_IN and POLL_OUT, as we don't know
the actual expected direction. But that's actually problematic. Consider
for example that write() ends with MBEDTLS_ERR_SSL_WANT_READ, but gets
converted to None. We put the underlying socket on pull using POLL_IN|POLL_OUT
but that probably returns immediately with POLL_OUT, as underlyings socket
is writable. We call the same ussl write() again, which again results in
MBEDTLS_ERR_SSL_WANT_READ, etc. We thus go into busy-loop.

So, the handling in this patch is temporary and needs fixing. But exact way
to fix it is not clear. One way is to provide explicit function for handshake
(CPython has do_handshake()), and let that return distinct codes like
WANT_READ/WANT_WRITE. But as mentioned above, past the initial handshake,
such situation may happen again with at least renegotiation. So apparently,
the only robust solution is to return "out of bound" special sentinels like
WANT_READ/WANT_WRITE from read()/write() directly. CPython throws exceptions
for these, but those are expensive to adopt that way for efficiency-conscious
implementation like MicroPython.

Change-Id: I440c67afcde1c5ca1bb20365408016924cb43149

pfalcon · 2019-02-08T10:10:58Z

These are initial steps to add non-blocking support to "ussl" module implementations. Non-blocking support for TLS isn't exactly trivial matter, and not all implementations do it right, for example, axTLS has issues with it. And implementations here try to establish basic API for enabling non-blocking mode, and this sits (and was being elaborated incrementally) for a while, so I'm sure these changes are in right direction, but some further changes will still be required. Anyway, I propose to merge these, to avoid possible duplication of work.

See also #3396 for some of the background discussion.

pfalcon · 2019-02-17T07:38:07Z

Ping @dpgeorge. "1 test failed" if only due to coverage decrease, due to need to disable very hackish test clauses in ussl test, the test overall having many issues: #4364

@Jongy: Please review.

Jongy · 2019-02-18T22:40:57Z

I'll look into it this week.

pfalcon · 2019-02-23T10:53:27Z

ping @dpgeorge

Jongy

About axTLS - I've never actually worked with this library before so I can't specifically comment. Nonetheless, the logic seems to match mbedtls which appears OK.

Jongy · 2019-02-24T21:22:39Z

extmod/modussl_mbedtls.c

@@ -184,12 +186,14 @@ STATIC mp_obj_ssl_socket_t *socket_new(mp_obj_t sock, struct ssl_args *args) {
        assert(ret == 0);
    }

+    if (args->do_handshake.u_bool) {
    while ((ret = mbedtls_ssl_handshake(&o->ssl)) != 0) {


I assume you want to keep the diff smaller, but I think you should indent here.

I assume you want to keep the diff smaller, but I think you should indent here.

Yeah, this comes from my branch, where I indeed try to minimize deltas to avoid conflicts. Fixed.

Jongy · 2019-02-24T21:28:34Z

extmod/modussl_axtls.c

+    mp_obj_t dest[3];
+    mp_load_method(sock, MP_QSTR_setblocking, dest);
+    dest[2] = flag_in;
+    return mp_call_method_n_kw(1, 0, dest);


I think you should first call mp_call_method_n_kw and only if it succeeds, then set o->blocking. Because if it fails at this point then the ssl socket is left in an unstable state.

I think you should first call mp_call_method_n_kw and only if it succeeds, then set o->blocking

Fixed.

Jongy · 2019-02-24T21:31:26Z

tests/extmod/ussl_basic.py

-    print('setblocking: NotImplementedError')
-ss.setblocking(True)
+# setblocking() propagates call to the underlying stream object, and
+# io.BytesIO doesn't have setblocking() (in CPython too).


Can't this test be implemented over loopback sockets? (Which I assume supported by most, if not all, ports supporting usocket in one way or another)

Can't this test be implemented over loopback sockets? (Which I assume supported by most, if not all, ports supporting usocket in one way or another)

I wouldn't make such assumption. Actually, ussl doesn't have much to do with sockets specifically, it's defined to operate on streams. So, bringing in sockets would only tangle the matter, making additional dependencies on sockets behavior (and support for loopback sockets shouldn't be taken for granted, and actually even presence of sockets shouldn't be).

For this, add wrap_socket(do_handshake=False) param. CPython doesn't have such a param at a module's global function, and at SSLContext.wrap_socket() it has do_handshake_on_connect param, but that uselessly long. Beyond that, make write() handle not just MBEDTLS_ERR_SSL_WANT_WRITE, but also MBEDTLS_ERR_SSL_WANT_READ, as during handshake, write call may be actually preempted by need to read next handshake message from peer. Likewise, for read(). And even after the initial negotiation, situations like that may happen e.g. with renegotiation. Both MBEDTLS_ERR_SSL_WANT_READ and MBEDTLS_ERR_SSL_WANT_WRITE are however mapped to the same None return code. The idea is that if the same read()/write() method is called repeatedly, the progress will be made step by step anyway. The caveat is if user wants to add the underlying socket to uselect.poll(). To be reliable, in this case, the socket should be polled for both POLL_IN and POLL_OUT, as we don't know the actual expected direction. But that's actually problematic. Consider for example that write() ends with MBEDTLS_ERR_SSL_WANT_READ, but gets converted to None. We put the underlying socket on pull using POLL_IN|POLL_OUT but that probably returns immediately with POLL_OUT, as underlyings socket is writable. We call the same ussl write() again, which again results in MBEDTLS_ERR_SSL_WANT_READ, etc. We thus go into busy-loop. So, the handling in this patch is temporary and needs fixing. But exact way to fix it is not clear. One way is to provide explicit function for handshake (CPython has do_handshake()), and let *that* return distinct codes like WANT_READ/WANT_WRITE. But as mentioned above, past the initial handshake, such situation may happen again with at least renegotiation. So apparently, the only robust solution is to return "out of bound" special sentinels like WANT_READ/WANT_WRITE from read()/write() directly. CPython throws exceptions for these, but those are expensive to adopt that way for efficiency-conscious implementation like MicroPython. Change-Id: I440c67afcde1c5ca1bb20365408016924cb43149

It consists of: 1. "do_handhake" param (default True) to wrap_socket(). If it's False, handshake won't be performed by wrap_socket(), as it would be done in blocking way normally. Instead, SSL socket can be set to non-blocking mode, and handshake would be performed before the first read/write request (by just returning EAGAIN to these requests, while instead reading/writing/processing handshake over the connection). Unfortunately, axTLS doesn't really support non-blocking handshake correctly. So, while framework for this is implemented on MicroPython's module side, in case of axTLS, it won't work reliably. 2. Implementation of .setblocking() method. It must be called on SSL socket for blocking vs non-blocking operation to be handled correctly (for example, it's not enough to wrap non-blocking socket with wrap_socket() call - resulting SSL socket won't be itself non-blocking). Note that .setblocking() propagates call to the underlying socket object, as expected. Change-Id: If3c9caa3bba57fddcc24a225b526c18338c20abd

Now that setblocking() is implemented in modussl_axtls, it calls into the underlying stream object, and io.BytesIO doesn't have setblocking(). Change-Id: I048879f05d092d9eecc99fb39d889a0b40959fa0

pfalcon · 2019-03-01T21:04:09Z

Ping @dpgeorge

dpgeorge · 2019-03-02T13:00:14Z

Thanks for this, it's a good direction to go in. My comments are:

CPython's ssl.wrap_socket() function (at the module level) does have the do_handshake_on_connect parameter (at least in CPython 3.7.2), so I don't see why it should be just do_handshake in uPy when they are the same parameter.
For the poll in/out issue: how does CPython handle this (does it handle it)? Would it work to register the ssl stream itself (instead of the underlying socket) with select/poll and then it has its own logic to work out if it is readable/writable?
In my understanding, if a file/stream is read for reading (ie a poll with POLLIN set returns ready) then the next call to read() must return some data, or at least must not return None (EAGAIN). Isn't that violated here, if read() won't yet return data because there needs to be some outgoing data first?

pfalcon · 2019-03-02T13:47:52Z

CPython's ssl.wrap_socket() function (at the module level) does have the do_handshake_on_connect parameter (at least in CPython 3.7.2), so I don't see why it should be just do_handshake in uPy when they are the same parameter.

But this is covered in the description:

CPython doesn't have
such a param at a module's global function, and at SSLContext.wrap_socket()
it has do_handshake_on_connect param, but that uselessly long.

Such a long name is a historical artifact, I don't think that it would go thru in the modern CPython. And as having too-long stuff is against MicroPython's approach, and we have ussl, not ssl, I don't think we should mimic historic artifacts of CPython.

For the poll in/out issue: how does CPython handle this (does it handle it)? Would it work to register the ssl stream itself (instead of the underlying socket) with select/poll and then it has its own logic to work out if it is readable/writable?

I'd like to leave this discussion until after this patch. This patch paves the road for non-blocking support for TLS, by dealing with the source of blocking-ness in ussl behavior, where initial handshake is expected to complete at once, i.e. in blocking in manner. Once that is resolved, we can discuss how to deal with polling ussl streams.

In my understanding, if a file/stream is read for reading (ie a poll with POLLIN set returns ready) then the next call to read() must return some data, or at least must not return None (EAGAIN). Isn't that violated here, if read() won't yet return data because there needs to be some outgoing data first?

That may be the rules for POSIX native fd's (I don't believe I ever read that formally specified, but that would be common expectation), but it does not hold for wrapper streams on top of native POSIX streams. This point is well established by now, it first occurred when implementing webrepl on top of websockets. I.e. in other words, when dealing with wrapper streams (and thus arbitrary streams in the general case), if user gets POLLIN from poll(), they shouldn't expect that call to read() won't return None (i.e. EAGAIN), likewise for write().

pfalcon · 2019-03-02T14:00:32Z

CPython's ssl.wrap_socket() function (at the module level) does have the do_handshake_on_connect parameter (at least in CPython 3.7.2)

Well, actually, checking closer, CPython ssl.wrap_socket(), i.e. module-level function, now described as taking do_handshake_on_connect=True down to 3.5 docs. Well, I dunno how to explain that. Either I really mixed up things, or it was a recent addition to e.g. 3.7, with backport to point releases for earlier versions.

But that doesn't invalidate the point that do_handshake_on_connect is very-very long name.

pfalcon · 2019-03-02T14:02:58Z

For completeness, might mention another alternative to implement non-blocking handshake:

Make wrap_socket() to never do handshake.
For blocking case, do handshake on 1st read()/write().
For non-blocking case, either do the same or add do_handshake() method (also in CPython).

But I decided it's more roundabout way, which only further departs from CPython behavior.

pfalcon · 2019-03-06T21:59:15Z

@dpgeorge: So, are there specific suggestions, or can this be merged?

dpgeorge · 2019-03-08T13:28:41Z

are there specific suggestions

For the do_handshake parameter please change it to the same as CPython, do_handshake_on_connect. If it has the same behaviour (which I assume it does, didn't check in detail) then it should have the same name.

pfalcon · 2019-03-09T20:45:02Z

For the do_handshake parameter please change it to the same as CPython, do_handshake_on_connect. If it has the same behaviour (which I assume it does, didn't check in detail) then it should have the same name.

But what about the claim that this is very, very, unreasonably long option name? The modern CPython does pay attention to the length of identifiers. To that effect, I can link to https://www.python.org/dev/peps/pep-0563/#making-the-name-of-the-future-import-more-verbose and quote it a bit:

the PEP could call the feature more explicitly, for example string_annotations, stringify_annotations, annotation_strings, annotations_as_strings, lazy_anotations, static_annotations, etc.

The problem with those names is that they are very verbose. Each of them besides lazy_annotations would constitute the longest future feature name in Python. They are long to type and harder to remember than the single-word form.

Based on accepted-PEP evaluation like above, it's fair to conjecture that an option like "do_handshake_on_connect" could not get into the modern CPython3 of today. And arguably, it got into in the first place by overlook of the reviewers.

Moreover, one of the possible courses of action for CPython might be providing a shorter name for that option, and deprecating the older one. Of course, they won't do that, because they're busy with other overly important stuff, but the point is that we don't have to repeat their mistakes. The proposed "do_handshake" option would behave similarly to CPython's "do_handshake_on_connect", but it's proposed for "ussl" module, whereas that option in CPython's "ssl" module. And "ussl" module as a whole is already different from CPython's "ssl" module, and would become only more different when non-blocking TLS operation is supported (to which this PR is a first step).

Summing up: The reason how "do_handshake_on_connect" got into CPython is by neglect of the fact that it's unwieldy long - by it's original author and contemporary reviewers. And I don't want to repeat the same mistake for MicroPython. And well, write-offs like "change it to the same as CPython, do_handshake_on_connect" seem like a neglect of all the issues described above. Now I don't want to say that it absolutely have to be named "do_handshake", but someone making a choice would need to weigh in and contrast approaches of "blindly follow what CPython does, no matter how mad it is, in the name of pseudo-compatibility (because we don't blindly follow it in many places already)" vs "try to fix up CPython warts in MicroPython API, because we have a good way to separate the two already".

And beyond that, there were other alternatives mentioned (#4481 (comment)), which apparently need response too to be able to say that this matter was considered from all sides.

And well, it would be nice to have that discussion done and move on. It's frustrating that issues like #4585 which could easily stay in queue for half a year, while this issue, which represents multi-year effort of implementing scalable scheme and best practices for generalized async I/O scheduling, gets so little attention.

peterhinch · 2019-04-23T07:02:52Z

It would be good to see progress on this.

dpgeorge · 2019-04-30T07:32:24Z

Ok, let's merge it then, with do_handshake.

Merged in 9c7c082, c764453, 7b54001

Switch devices.h to nvm.toml data

pfalcon changed the title ~~extmod/modussl_mbedtls: Support non-blocking handshake.~~ extmod/modussl*: Initial support for non-blocking mode. Feb 8, 2019

Jongy suggested changes Feb 24, 2019

View reviewed changes

pfalcon force-pushed the ussl-nonblock branch 2 times, most recently from d702b4e to bcdf1ca Compare March 1, 2019 21:02

Paul Sokolovsky added 3 commits March 2, 2019 00:03

tests/ussl_basic: Disable setblocking() calls.

1ce7ad8

Now that setblocking() is implemented in modussl_axtls, it calls into the underlying stream object, and io.BytesIO doesn't have setblocking(). Change-Id: I048879f05d092d9eecc99fb39d889a0b40959fa0

pfalcon force-pushed the ussl-nonblock branch from bcdf1ca to 1ce7ad8 Compare March 1, 2019 21:03

peterhinch mentioned this pull request Mar 4, 2019

SSL issues peterhinch/micropython-mqtt#10

Closed

dpgeorge closed this Apr 30, 2019

tannewt added a commit to tannewt/circuitpython that referenced this pull request Mar 31, 2021

Merge pull request micropython#4481 from tannewt/external_devices

5810461

Switch devices.h to nvm.toml data

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

extmod/modussl*: Initial support for non-blocking mode. #4481

extmod/modussl*: Initial support for non-blocking mode. #4481

pfalcon commented Feb 8, 2019

pfalcon commented Feb 8, 2019

pfalcon commented Feb 17, 2019

Jongy commented Feb 18, 2019

pfalcon commented Feb 23, 2019

Jongy left a comment

Jongy Feb 24, 2019

pfalcon Mar 1, 2019

Jongy Feb 24, 2019

pfalcon Mar 1, 2019

Jongy Feb 24, 2019

pfalcon Mar 1, 2019

pfalcon commented Mar 1, 2019

dpgeorge commented Mar 2, 2019

pfalcon commented Mar 2, 2019

pfalcon commented Mar 2, 2019

pfalcon commented Mar 2, 2019

pfalcon commented Mar 6, 2019

dpgeorge commented Mar 8, 2019

pfalcon commented Mar 9, 2019

peterhinch commented Apr 23, 2019

dpgeorge commented Apr 30, 2019

extmod/modussl*: Initial support for non-blocking mode. #4481

extmod/modussl*: Initial support for non-blocking mode. #4481

Conversation

pfalcon commented Feb 8, 2019

pfalcon commented Feb 8, 2019

pfalcon commented Feb 17, 2019

Jongy commented Feb 18, 2019

pfalcon commented Feb 23, 2019

Jongy left a comment

Choose a reason for hiding this comment

Jongy Feb 24, 2019

Choose a reason for hiding this comment

pfalcon Mar 1, 2019

Choose a reason for hiding this comment

Jongy Feb 24, 2019

Choose a reason for hiding this comment

pfalcon Mar 1, 2019

Choose a reason for hiding this comment

Jongy Feb 24, 2019

Choose a reason for hiding this comment

pfalcon Mar 1, 2019

Choose a reason for hiding this comment

pfalcon commented Mar 1, 2019

dpgeorge commented Mar 2, 2019

pfalcon commented Mar 2, 2019

pfalcon commented Mar 2, 2019

pfalcon commented Mar 2, 2019

pfalcon commented Mar 6, 2019

dpgeorge commented Mar 8, 2019

pfalcon commented Mar 9, 2019

peterhinch commented Apr 23, 2019

dpgeorge commented Apr 30, 2019