Set SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER in calling OpenSSL by julianz- · Pull Request #1287 · pyca/pyopenssl

julianz- · 2024-01-25T20:13:43Z

See cherrypy/cheroot#245 for discussion.

mhils · 2024-01-25T20:31:58Z

See #1242 for more context.

mhils

Thanks for pushing this further! 🍰 I think we had general agreement to do this in #1242, so this looks good to merge after some minor docs fixes. :)

We can ignore codecov, that seems to be a bug in determining coverage.

webknjaz

Add some empty line separators under title marks.

webknjaz · 2024-01-28T19:39:05Z

@mhils @alex it appears that https://app.codecov.io/gh/pyca/pyopenssl treats master as the default branch, which is why it may get confused and assign coverage drops to pull requests that have nothing to do with said lines.

alex · 2024-01-28T19:42:42Z

I've changed the default branch to be main

webknjaz · 2024-01-28T22:23:08Z

Is there a smoke test we could include in this PR?

julianz- · 2024-01-29T04:17:06Z

@webknjaz The only test for setting the mode currently is:

def test_set_mode(self):
        """
        `Context.set_mode` accepts a mode bitvector and returns the
        newly set mode.
        """
        context = Context(SSLv23_METHOD)
        assert MODE_RELEASE_BUFFERS & context.set_mode(MODE_RELEASE_BUFFERS)

This checks that setting the mode for MODE_RELEASE_BUFFERS returns the same bit. I guess we could add another check to make sure passing SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER also returns the appropriate value? Not sure whether that counts as a smoke test but it's something perhaps?

webknjaz · 2024-01-29T14:36:31Z

Maybe, find some existing test where a write buffer is passed, copy it and pass a moving buffer there?

@mhils ideas?

mhils · 2024-01-29T14:47:30Z

We could possibly adapt something like this: https://github.com/pyca/pyopenssl/blob/main/tests/test_ssl.py#L2837

If that's easy to add I'm all for it, but I also feel that not having an elaborate test here is not the end of the world. There's precedent (SSL_MODE_ENABLE_PARTIAL_WRITE has no test either) and, more importantly, if this fails in the future we should get a very explicit 'bad write retry' error. Does CPython have a dedicated test for this?

This looks good to merge otherwise. @alex @reaperhulk, if you want to make a judgement call here please just merge. :)

julianz- · 2024-01-29T20:12:24Z

@mhils Strangely, when I try running pytest locally on my branch I am getting an exception on that test that makes it fail the test:

E       OpenSSL.SSL.Error: [('system library', '', ''), ('system library', '', ''), ('system library', '', ''), ('system library', '', ''), ('SSL routines', '', 'certificate verify failed')]

../../../../Library/Python/3.9/lib/python/site-packages/OpenSSL/_util.py:57: Error
____________________________________________________________________ TestConnection.test_wantWriteError _____________________________________________________________________

That's just a fragment of the output so maybe not very meaningful but as I understand it, the test is meant to throw WantWriteError but for some reason although it's expected the exception is not being considered a success? How is this supposed to work?

webknjaz · 2024-01-29T20:59:58Z

although it's expected the exception is not being considered a success?

In your log, a more generic exception happens (OpenSSL.SSL.Error), not OpenSSL.SSL.WantWriteError. This is why pytest.raises() doesn't match it as expected.

julianz- · 2024-02-02T00:41:14Z

@webknjaz

In your log, a more generic exception happens (OpenSSL.SSL.Error), not OpenSSL.SSL.WantWriteError. This is why pytest.raises() doesn't match it as expected.

Ok to make any progress on this, I'm trying to understand the first test that is failing when I run pytest locally - test_set_default_verify_paths() in test_ssl.py. It's basically saying "certificate verify failed". I tried debugging this using the following in the command line:

openssl s_client -connect "encrypted.google.com":443

and get:

Verify return code: 18 (self signed certificate)

I assume this generates an error because the return code is not 0. But how is this supposed to work? The test relies on Google's certificate but there seems to be problem with the cert?

alex

Very close! Thanks for your persistence on this.

I'm a bit ambivalent on the logs, it's not really our house style and IME for things like this, while the logs are useful in the initial development, they're unlikely to be useful down the line. (In contrast to logs in production software, where you have a more continious validation process.)

alex · 2025-08-14T03:06:36Z

+        try:
+            if server:
+                server.shutdown()
+        except Exception:  # pragma: no cover


Same comment as above

webknjaz · 2025-08-15T13:21:55Z

+    def _badwriteretry(
+        self,
+        want_bad_retry: bool,
+        modeflag: int | None,
+    ) -> bool:
+        """
+        Tests for a "bad write retry" error over an SSL connection
+        by using a moving buffer which is allowed by default with
+        SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER. If this mode is unset
+        we expect a bad write retry error when using a moving buffer.
+        want_bad_retry: If True, the caller expects a bad write retry error.
+        mode: If not None, unsets the defaults that include
+        SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER and replaces with the given mode.
+        Returns True if a bad write retry error occurs.
+        """
+        request_buffer_size = 65536  # Size of the send buffer we'll request
+        client_socket, server_socket, client, server, sndbuf, rcvbuf = (
+            create_ssl_nonblocking_connection(modeflag, request_buffer_size)
+        )
+        result = False  # Default return value
+        # set buffer size to half the minimum of send and receive buffers
+        buffer_size = min(sndbuf, rcvbuf) // 2
+
+        # --- Main Test Flow ---
+        # _attempt_want_write_error() terminates the test
+        # if WantWriteError is not triggered
+        # The function also returns the message that triggered
+        # the WantWriteError so that when we attempt a retry
+        # we can ensure a different buffer location is allocated
+        # to a the new message we will send for the retry.
+        _ = self._attempt_want_write_error(client, buffer_size)
+
+        # proceed with draining so that a retry has a chance to succeed
+        self._drain_server_buffers(server, server_socket)
+
+        # now attempt the moving buffer retry
+        result = self._perform_moving_buffer_test(
+            client, buffer_size, want_bad_retry
+        )
+        self._shutdown_connections(
+            client, server, client_socket, server_socket
+        )
+        return result


This looks like a sort of thing that seems natural to wrap with @contextlib.contextmanager or (even better @pytest.fixture with a yield for cleanup and indirect params).

Thanks for the suggestion will do.

…WRITE errors. When SSL_WANT_READ or SSL_WANT_WRITE are encountered, it's typical to retry the call but this must be repeated with the exact same arguments. Without this change, openSSL requires that the address of the buffer passed is the same. However, buffers in python can change location in some circumstances which cause the retry to fail. By add the setting SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER, the requirement for the same buffer address is forgiven and the retry has a better chance of success. See cherrypy/cheroot#245 for discussion.

alex · 2025-08-16T17:33:09Z

There's some lint errors, and you'll need to update the minimum version in noxfile.py, but the code itself looks good. Thanks for your persistence herer!

This reverts commit bb1d143.

alex

Thank you!

julianz- · 2025-08-16T22:27:35Z

Thank you @alex and @webknjaz!

webknjaz · 2025-08-17T10:19:33Z

Thanks! Hopefully, this will unblock Cheroot once released..

julianz- · 2025-08-17T16:58:49Z

Thanks! Hopefully, this will unblock Cheroot once released.

Agreed! I wanted to create a PR for Cheroot once we had this.

webknjaz · 2025-09-20T23:29:18Z

Thanks! Hopefully, this will unblock Cheroot once released.

Agreed! I wanted to create a PR for Cheroot once we had this.

It's been released a week ago. So that's finally unblocked!

julianz- · 2025-09-20T23:42:04Z

Great thanks for the heads up!

mhils mentioned this pull request Jan 25, 2024

Fix problem caused by SSL_WANT_READ or SSL_WANT_WRITE errors. #1242

Closed

mhils reviewed Jan 25, 2024

View reviewed changes

Comment thread CHANGELOG.rst Outdated

Comment thread CHANGELOG.rst Outdated

mhils reviewed Jan 25, 2024

View reviewed changes

Comment thread CHANGELOG.rst Outdated