Set SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER in calling OpenSSL

[julianz-]

See cherrypy/cheroot#245 for discussion.

[mhils]

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]

@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]

I've changed the default branch to be main

[webknjaz]

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

[julianz-]

@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]

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

@mhils ideas?

[mhils]

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-]

@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]

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-]

@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?

[julianz-]

@alex

I think to get this merged, the following need to happen:

• Dropping support for Python versions or anything like that should not be in this PR
• I don't understand why several tests were removed? Those seem orthagonal to what I understand this PR to do
• We need a test that shows the desired behavior with this PR (not just that the flag is set, but that the object behaves correctly.)

I have updated this PR, restoring the tests that were previously failing (your second point). I have also added two tests that check for the use of SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER, one for its presence and one for its absence (your third point). I still have had to remove py37-cryptographyMain from the CI matrix as it I got conflicts with the requirements in setup.py for cryptography that were already there:
(
"typing-extensions>=4.9; "
"python_version < '3.13' and python_version >= '3.8'"
),

If you still want the removal of py37-cryptographyMain in separate PR let me know.

[julianz-]

@alex @webknjaz @mhils
Just checking on review status? I just recently updated this PR quite extensively.

[alex]

I am to get to it this weekend.