On Mon, May 13, 2019 at 03:18:14PM +0200, Guilhem Moulin <guil...@debian.org> wrote: > On Mon, 13 May 2019 at 06:31:26 +0200, Steffen Ullrich wrote: > > Applications which relied on blocking I/O in connection with select could > > also hang before, > > Uh, what? “Before” meaning with ≤TLSv1.2, or with OpenSSL <1.1.1a's > default flags? libssl mentions no such thing beside the new default > mode. And in fact the s_client() program, *from OpenSSL upstream > itself*, does precisely that: a select loop in blocking I/O mode (unless > the ‘-nbio’ flag is set).
"Before" meaning already with previous OpenSSL versions. But the effect was likely hard to notice: an incomplete SSL record would not cause a permanent hang but only a blocking read until the rest of the data was received, i.e. only some slow down in some situations. > > > https://github.com/openssl/openssl/blob/OpenSSL_1_1_1/apps/s_client.c#L2817 > > s_client() is roughly speaking a C version the ‘netcat.pl’ prototype I > attached earlier. Unsurprisingly, since 1.1.1a the code clears > SSL_MODE_AUTO_RETRY from the bitmask mode of the newly created SSL_CTX > object. That change was even done in the very same commit that enabled > SSL_MODE_AUTO_RETRY by default :-) > > > https://github.com/openssl/openssl/commit/693cf80c6ff54ae276a44d305d4ad07168ec6895#diff-7f3b79983f6d53c047c90a62813cc11f s_client is just a better test program and not production code which actually cares about edge cases. And the change was likely needed since s_client now had serious problems and this was the easiest way to address these. > IMHO using polling sockets in blocking I/O is fairly common. Granted, > that itself itself doesn't say much about code quality, but the fact that > the OpenSSL project *does* use it (and explicitly mentions it in the > docs) gives me confidence that it's a fine use which should still be > supported. From my experience with OpenSSL and its documentation I don't agree with this. Documentation is sometimes wrong or misleading and more often incomplete (but its actually improving). And while using blocking I/O with polling sockets might be fine with plain sockets where each byte is part of application data it is not fine with SSL since the unit in SSL is not a byte but a record and some records might contain application data and some not. > The documentation (libssl's SSL_read()'s [0], Net::SSLeay::read() [1], > as well as IO::Socket::SSL::sysread() [2]) all warn that reading from an > SSL socket has a different behavior than usual read(2) system calls, in > that read failures should be treated with care, as there are both > retryable (eg. when no application data was received) and non-retryable > errors. As I was the one who wrote the documentation for IO::Socket::SSL: the part about sysread failing was supposed to be about non-blocking sockets. A blocking socket was not expected to return nothing just because the handshake was not done. > > So from my perspective, the expectation that IO::Socket::SSL::sysread() > behaves like a “normal” sysread doesn't hold, and never did. (There is > even an entry for “Expecting exactly the same behavior as plain > sockets” in the “Common errors” section of IO::Socket::SSL's manpage!) This part talks about specific cases and how to deal with these but says nothing about blocking sockets temporary failing. > For what it's worth I think it's a shame that SSL_MODE_AUTO_RETRY was > not the default earlier, as it's convenient to be able to write > > $sock->sysread($buf, $len) // die; > > and not bother about inspecting $SSL_ERROR. But programs have been I agree with that. But in the past SSL_MODE_AUTO_RETRY was only relevant with unexpected renegotiations which nearly never happened. So nobody actually noticed. > written with the old default in mind, and the fact that SSL_read() > doesn't behave like a normal read() system call. For these programs to > keep working, there needs to be a way to switch SSL_MODE_AUTO_RETRY off. > libssl provides SSL_CTX_clear_mode(), Net::SSLeay has > CTX_ctrl(,78,SSL_CTRL_CLEAR_MODE,0), and I'd like to have something > similar in IO::Socket::SSL, too. I'm not convinced that this is the way to go, i.e. I'd rather see the applications use non-blocking sockets. But I'm also not stubbornly against it. Do you know a relevant module or actually used application which has problems because of the default mode of SSL_MODE_AUTO_RETRY the ability to change it would fix the related problems and not introduce new ones, i.e. where such a fix would be the right thing to do? LWP of course does not count since the right thing to do in LWP was to remove the obsolete patch which kept the socket blocking. I have also some problems to provide a consistent API for this which can actually be understood. This would in my opinion mean that I have a clear default for the value, i.e. that I enable SSL_MODE_AUTO_RETRY by default for older OpenSSL versions the same way it is done in OpenSSL 1.1.1. But that might break other things :( Another option might be to have an option SSL_mode_auto_retry which by default is not set but might be set to either 0 or 1 to get a consistent behavior across different OpenSSL versions. Regards, Steffen
