pyopenssl
pyopenssl copied to clipboard
pyca
WANT_WRITE_ERROR occurs for sendall
The OpenSSL documentation says that in the event of a WANT_WRITE_ERROR or WANT_READ_ERROR, the same OpenSSL method call is to be repeated, otherwise you will get a bad write retry error.
See here: https://www.openssl.org/docs/ssl/SSL_write.html http://stackoverflow.com/questions/2997218/why-am-i-getting-error1409f07fssl-routinesssl3-write-pending-bad-write-retr
For pyOpenSSL.sendAll() this is problematic. Because python's sendAll() call does not return the number of bytes already sent if an error is thrown, it's impossible to continue with an identical SSL.Write() call. The state of the last SSL.Write() call is only known inside the pyOpenSSL.sendAll() function, and since this function has thrown an exception, this state is lost.
To illustrate the problem, see the following source for SSL.sendall() in pyOpenSSL:
left_to_send = len(buf)
total_sent = 0
data = _ffi.new("char[]", buf)
while left_to_send:
result = _lib.SSL_write(self._ssl, data + total_sent, left_to_send)
self._raise_ssl_error(self._ssl, result)
total_sent += result
left_to_send -= result
Specifically, inside the while loop, there are repeated calls to _lib.SSL_write(). _lib is the openSSL library, so the correct contract is that if SSL_write throws a WantWriteError or WantReadError, it should be handled by sleeping for a short duration and re-issuing the same call. However, pyOpenSSL does not do this, and the caller cannot either.
As a workaround, I implemented a wrapper around pyOpenSSL, and implemented my own sendAll() to handle any WANT_WRITE_ERROR or WANT_READ_ERRORS in a loop, re-issuing the same send() call that the error threw (after a small sleep).
Finally, keep in mind that any write or read calls that are made to pyOpenSSL can fail with either a WANT_WRITE_ERROR or a WANT_READ_ERROR, because these calls could trigger an SSL handshake. If network buffers are full or nearly full when the handshake triggers, these errors can be thrown. For this reason, both error conditions must be handled by all network calls, either in pyOpenSSL or by developer using pyOpenSSL.
Recommended fix: pyOpenSSL should write exception handlers to catch WANT_WRITE_ERROR and WANT_READ_ERRORS inside all functions to catch those exceptions, sleep for a small period of time, and re-issue the same SSL_write() or SSL_read() call. The specific implementation will need to be considerate of blocking vs non-blocking mode, as well as timeouts, and is likely not trivial.
Thank you for all the work you do on pyOpenSSL. It really is a fantastic library.
pyOpenSSL should write exception handlers to catch WANT_WRITE_ERROR and WANT_READ_ERRORS inside all functions to catch those exceptions
Can you explain why you think this needs to be applied to all methods, not just sendall?
exarkun: sorry--I did not see this for a long time.
sendall needs the fix, as it's impossible for the user to retry the call as described above. For the rest of the methods, the user can retry the call manually, but it makes the userspace code uglier. It would be ideal if the library handled such conditions. It would also be nice to be consistent between the various methods (send vs sendall).
However, if you chose not to handle these errors, the userspace code CAN handle them.
For the rest of the methods, the user can retry the call manually, but it makes the userspace code uglier.
pyOpenSSL is meant as a fairly low-level interface. It supports a variety of use-cases - for example, non-blocking TLS. That use-case would be impeded by automatic retries in send and recv. I suspect that non-blocking code is already prevented from using sendall so fixing it there seems like it could be reasonable. I don't think the change should be applied elsewhere, though. User code that doesn't want to deal with such low-level details should use a higher-level library (eg Twisted).
Understood. Thanks for your time. We've implemented a workaround in sendall for our wrapper, so we're all set with this issue.
Actually, oops. Leaving this open because I still think doing retry in sendall in pyOpenSSL would be fine. :smile:
Looks like the patch with retries needs to be applied around these lines: https://github.com/pyca/pyopenssl/blob/124a013/src/OpenSSL/SSL.py#L1670-L1673
Per https://www.openssl.org/docs/man1.1.1/man3/SSL_write_ex.html, sendall() should use _lib.SSL_write_ex() instead of _lib.SSL_write() so that it's possible to figure out how many bytes have been written by a failed operation.
But it looks like it's not exposed via cryptography so that should probably happen first: https://github.com/pyca/cryptography/blob/b0a3d89e0f69d6e460a4ae65a57ea2c721f9370b/src/_cffi_src/openssl/ssl.py#L167.
Hm... This https://github.com/pyca/pyopenssl/blob/124a013/src/OpenSSL/SSL.py#L1623-L1625 suggests that SSL_write() could be retried with the same buffer, although I don't understand why.
So looking at the source, it seems like SSL_MODE_ENABLE_PARTIAL_WRITE is set and I think calls to SSL_write() return the bytes written when at least 1 byte has been sent. This means that we can expect that if self._raise_ssl_error(self._ssl, result) raises an error, then 0 bytes have been sent.
I think that the patch would roughly be wrapping https://github.com/pyca/pyopenssl/blob/124a0134fdb7decb0136b4b6f7892b87b919e74e/src/OpenSSL/SSL.py#L1673 with try/except and doing a retry:
def sendall(self, buf, flags=0):
"""
Send "all" data on the connection. This calls send() repeatedly until
all data is sent. If an error occurs, it's impossible to tell how much
data has been sent.
:param buf: The string, buffer or memoryview to send
:param flags: (optional) Included for compatibility with the socket
API, the value is ignored
:return: The number of bytes written
"""
buf = _text_to_bytes_and_warn("buf", buf)
with _from_buffer(buf) as data:
left_to_send = len(buf)
total_sent = 0
while left_to_send:
# SSL_write's num arg is an int,
# so we cannot send more than 2**31-1 bytes at once.
result = _lib.SSL_write(
self._ssl, data + total_sent, min(left_to_send, 2147483647)
)
- self._raise_ssl_error(self._ssl, result)
+ try:
+ self._raise_ssl_error(self._ssl, result)
+ except (WantReadError, WantWriteError):
+ continue
total_sent += result
left_to_send -= result
return total_sent
I don't think the change should be applied elsewhere, though.
@exarkun FTR CPython implements retries in their send() implementation: https://github.com/python/cpython/blob/c32f2976b8f4034724c3270397aa16f38daf470f/Modules/_ssl.c#L2467-L2468
It's implemented without the use of SSL_MODE_ENABLE_PARTIAL_WRITE — they have an immutable buffer that they keep passing into SSL_write() unchanged and the internal machinery in OpenSSL keeps a pointer to the slice that's still unsent.
And their sendall() also does that with a memoryview object:
https://github.com/python/cpython/blob/c32f2976b8f4034724c3270397aa16f38daf470f/Lib/ssl.py#L1193-L1207
CPython's approach may not be the best approach. Some decisions and workarounds pre-date OpenSSL 0.9.8.
I'm not too involved in pyOpenSSL development anymore (and haven't been for years). But I think it would be great if this PR were augmented with automated tests demonstrating the desired behavior has been achieved.
@exarkun I wish. Here's my attempt: https://github.com/pyca/pyopenssl/pull/955. But when applying it to implementation in the PR I get ssl3_read_bytes and I'm not sure what I'm doing wrong..