Set SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER in calling OpenSSL #1287

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.

CHANGELOG.rst

tests/test_crypto.py

src/OpenSSL/SSL.py

CHANGELOG.rst

webknjaz

Add some empty line separators under title marks.

CHANGELOG.rst

setup.py

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.

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?

webknjaz · 2024-03-18T15:02:33Z

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?

Yes, it is customary to interpret any non-zero return code as an error. I believe, this convention originated in POSIX.

Not sure how you got a self-signed certificate, though. Have you set up your DNS / /etc/hosts to remap the domain to something else?

The actual host responds with the correct certificate and the handshake succeeds:

$ openssl s_client -connect "encrypted.google.com":443
CONNECTED(00000004)
depth=2 C = US, O = Google Trust Services LLC, CN = GTS Root R1
verify return:1
depth=1 C = US, O = Google Trust Services LLC, CN = GTS CA 1C3
verify return:1
depth=0 CN = *.google.com
verify return:1
---
Certificate chain
 0 s:CN = *.google.com
   i:C = US, O = Google Trust Services LLC, CN = GTS CA 1C3
   a:PKEY: id-ecPublicKey, 256 (bit); sigalg: RSA-SHA256
   v:NotBefore: Feb 19 08:03:54 2024 GMT; NotAfter: May 13 08:03:53 2024 GMT
 1 s:C = US, O = Google Trust Services LLC, CN = GTS CA 1C3
   i:C = US, O = Google Trust Services LLC, CN = GTS Root R1
   a:PKEY: rsaEncryption, 2048 (bit); sigalg: RSA-SHA256
   v:NotBefore: Aug 13 00:00:42 2020 GMT; NotAfter: Sep 30 00:00:42 2027 GMT
 2 s:C = US, O = Google Trust Services LLC, CN = GTS Root R1
   i:C = BE, O = GlobalSign nv-sa, OU = Root CA, CN = GlobalSign Root CA
   a:PKEY: rsaEncryption, 4096 (bit); sigalg: RSA-SHA256
   v:NotBefore: Jun 19 00:00:42 2020 GMT; NotAfter: Jan 28 00:00:42 2028 GMT
---
Server certificate
-----BEGIN CERTIFICATE-----
MIIOXTCCDUWgAwIBAgIRAJF3iEqsDpoECedLdRgGyygwDQYJKoZIhvcNAQELBQAw
RjELMAkGA1UEBhMCVVMxIjAgBgNVBAoTGUdvb2dsZSBUcnVzdCBTZXJ2aWNlcyBM
TEMxEzARBgNVBAMTCkdUUyBDQSAxQzMwHhcNMjQwMjE5MDgwMzU0WhcNMjQwNTEz
MDgwMzUzWjAXMRUwEwYDVQQDDAwqLmdvb2dsZS5jb20wWTATBgcqhkjOPQIBBggq
hkjOPQMBBwNCAAQ7UkR1HmtagYbBBsPVqVik2xZO85oyD4jqvtxb2zKUdBCN3n+E
YFxtMs4KiRDvMzp3C/xWfNaMoF3X7/sDUm3ro4IMPjCCDDowDgYDVR0PAQH/BAQD
AgeAMBMGA1UdJQQMMAoGCCsGAQUFBwMBMAwGA1UdEwEB/wQCMAAwHQYDVR0OBBYE
FO620PFJar/4etLB8PcVeSgTJybuMB8GA1UdIwQYMBaAFIp0f6+Fze6VzT2c0OJG
FPNxNR0nMGoGCCsGAQUFBwEBBF4wXDAnBggrBgEFBQcwAYYbaHR0cDovL29jc3Au
cGtpLmdvb2cvZ3RzMWMzMDEGCCsGAQUFBzAChiVodHRwOi8vcGtpLmdvb2cvcmVw
by9jZXJ0cy9ndHMxYzMuZGVyMIIJ7wYDVR0RBIIJ5jCCCeKCDCouZ29vZ2xlLmNv
bYIWKi5hcHBlbmdpbmUuZ29vZ2xlLmNvbYIJKi5iZG4uZGV2ghUqLm9yaWdpbi10
ZXN0LmJkbi5kZXaCEiouY2xvdWQuZ29vZ2xlLmNvbYIYKi5jcm93ZHNvdXJjZS5n
b29nbGUuY29tghgqLmRhdGFjb21wdXRlLmdvb2dsZS5jb22CCyouZ29vZ2xlLmNh
ggsqLmdvb2dsZS5jbIIOKi5nb29nbGUuY28uaW6CDiouZ29vZ2xlLmNvLmpwgg4q
Lmdvb2dsZS5jby51a4IPKi5nb29nbGUuY29tLmFygg8qLmdvb2dsZS5jb20uYXWC
DyouZ29vZ2xlLmNvbS5icoIPKi5nb29nbGUuY29tLmNvgg8qLmdvb2dsZS5jb20u
bXiCDyouZ29vZ2xlLmNvbS50coIPKi5nb29nbGUuY29tLnZuggsqLmdvb2dsZS5k
ZYILKi5nb29nbGUuZXOCCyouZ29vZ2xlLmZyggsqLmdvb2dsZS5odYILKi5nb29n
bGUuaXSCCyouZ29vZ2xlLm5sggsqLmdvb2dsZS5wbIILKi5nb29nbGUucHSCDyou
Z29vZ2xlYXBpcy5jboIRKi5nb29nbGV2aWRlby5jb22CDCouZ3N0YXRpYy5jboIQ
Ki5nc3RhdGljLWNuLmNvbYIPZ29vZ2xlY25hcHBzLmNughEqLmdvb2dsZWNuYXBw
cy5jboIRZ29vZ2xlYXBwcy1jbi5jb22CEyouZ29vZ2xlYXBwcy1jbi5jb22CDGdr
ZWNuYXBwcy5jboIOKi5na2VjbmFwcHMuY26CEmdvb2dsZWRvd25sb2Fkcy5jboIU
Ki5nb29nbGVkb3dubG9hZHMuY26CEHJlY2FwdGNoYS5uZXQuY26CEioucmVjYXB0
Y2hhLm5ldC5jboIQcmVjYXB0Y2hhLWNuLm5ldIISKi5yZWNhcHRjaGEtY24ubmV0
ggt3aWRldmluZS5jboINKi53aWRldmluZS5jboIRYW1wcHJvamVjdC5vcmcuY26C
EyouYW1wcHJvamVjdC5vcmcuY26CEWFtcHByb2plY3QubmV0LmNughMqLmFtcHBy
b2plY3QubmV0LmNughdnb29nbGUtYW5hbHl0aWNzLWNuLmNvbYIZKi5nb29nbGUt
YW5hbHl0aWNzLWNuLmNvbYIXZ29vZ2xlYWRzZXJ2aWNlcy1jbi5jb22CGSouZ29v
Z2xlYWRzZXJ2aWNlcy1jbi5jb22CEWdvb2dsZXZhZHMtY24uY29tghMqLmdvb2ds
ZXZhZHMtY24uY29tghFnb29nbGVhcGlzLWNuLmNvbYITKi5nb29nbGVhcGlzLWNu
LmNvbYIVZ29vZ2xlb3B0aW1pemUtY24uY29tghcqLmdvb2dsZW9wdGltaXplLWNu
LmNvbYISZG91YmxlY2xpY2stY24ubmV0ghQqLmRvdWJsZWNsaWNrLWNuLm5ldIIY
Ki5mbHMuZG91YmxlY2xpY2stY24ubmV0ghYqLmcuZG91YmxlY2xpY2stY24ubmV0
gg5kb3VibGVjbGljay5jboIQKi5kb3VibGVjbGljay5jboIUKi5mbHMuZG91Ymxl
Y2xpY2suY26CEiouZy5kb3VibGVjbGljay5jboIRZGFydHNlYXJjaC1jbi5uZXSC
EyouZGFydHNlYXJjaC1jbi5uZXSCHWdvb2dsZXRyYXZlbGFkc2VydmljZXMtY24u
Y29tgh8qLmdvb2dsZXRyYXZlbGFkc2VydmljZXMtY24uY29tghhnb29nbGV0YWdz
ZXJ2aWNlcy1jbi5jb22CGiouZ29vZ2xldGFnc2VydmljZXMtY24uY29tghdnb29n
bGV0YWdtYW5hZ2VyLWNuLmNvbYIZKi5nb29nbGV0YWdtYW5hZ2VyLWNuLmNvbYIY
Z29vZ2xlc3luZGljYXRpb24tY24uY29tghoqLmdvb2dsZXN5bmRpY2F0aW9uLWNu
LmNvbYIkKi5zYWZlZnJhbWUuZ29vZ2xlc3luZGljYXRpb24tY24uY29tghZhcHAt
bWVhc3VyZW1lbnQtY24uY29tghgqLmFwcC1tZWFzdXJlbWVudC1jbi5jb22CC2d2
dDEtY24uY29tgg0qLmd2dDEtY24uY29tggtndnQyLWNuLmNvbYINKi5ndnQyLWNu
LmNvbYILMm1kbi1jbi5uZXSCDSouMm1kbi1jbi5uZXSCFGdvb2dsZWZsaWdodHMt
Y24ubmV0ghYqLmdvb2dsZWZsaWdodHMtY24ubmV0ggxhZG1vYi1jbi5jb22CDiou
YWRtb2ItY24uY29tghRnb29nbGVzYW5kYm94LWNuLmNvbYIWKi5nb29nbGVzYW5k
Ym94LWNuLmNvbYIeKi5zYWZlbnVwLmdvb2dsZXNhbmRib3gtY24uY29tgg0qLmdz
dGF0aWMuY29tghQqLm1ldHJpYy5nc3RhdGljLmNvbYIKKi5ndnQxLmNvbYIRKi5n
Y3BjZG4uZ3Z0MS5jb22CCiouZ3Z0Mi5jb22CDiouZ2NwLmd2dDIuY29tghAqLnVy
bC5nb29nbGUuY29tghYqLnlvdXR1YmUtbm9jb29raWUuY29tggsqLnl0aW1nLmNv
bYILYW5kcm9pZC5jb22CDSouYW5kcm9pZC5jb22CEyouZmxhc2guYW5kcm9pZC5j
b22CBGcuY26CBiouZy5jboIEZy5jb4IGKi5nLmNvggZnb28uZ2yCCnd3dy5nb28u
Z2yCFGdvb2dsZS1hbmFseXRpY3MuY29tghYqLmdvb2dsZS1hbmFseXRpY3MuY29t
ggpnb29nbGUuY29tghJnb29nbGVjb21tZXJjZS5jb22CFCouZ29vZ2xlY29tbWVy
Y2UuY29tgghnZ3BodC5jboIKKi5nZ3BodC5jboIKdXJjaGluLmNvbYIMKi51cmNo
aW4uY29tggh5b3V0dS5iZYILeW91dHViZS5jb22CDSoueW91dHViZS5jb22CFHlv
dXR1YmVlZHVjYXRpb24uY29tghYqLnlvdXR1YmVlZHVjYXRpb24uY29tgg95b3V0
dWJla2lkcy5jb22CESoueW91dHViZWtpZHMuY29tggV5dC5iZYIHKi55dC5iZYIa
YW5kcm9pZC5jbGllbnRzLmdvb2dsZS5jb22CG2RldmVsb3Blci5hbmRyb2lkLmdv
b2dsZS5jboIcZGV2ZWxvcGVycy5hbmRyb2lkLmdvb2dsZS5jboIYc291cmNlLmFu
ZHJvaWQuZ29vZ2xlLmNughpkZXZlbG9wZXIuY2hyb21lLmdvb2dsZS5jboIYd2Vi
LmRldmVsb3BlcnMuZ29vZ2xlLmNuMCEGA1UdIAQaMBgwCAYGZ4EMAQIBMAwGCisG
AQQB1nkCBQMwPAYDVR0fBDUwMzAxoC+gLYYraHR0cDovL2NybHMucGtpLmdvb2cv
Z3RzMWMzL2ZWSnhiVi1LdG1rLmNybDCCAQMGCisGAQQB1nkCBAIEgfQEgfEA7wB1
ADtTd3U+LbmAToswWwb+QDtn2E/D9Me9AA0tcm/h+tQXAAABjcCbl6wAAAQDAEYw
RAIgSsPqmrwU1+TmKxlq0lWFp8HhAWMNr8TpdCsZZ2hIZIQCIHHVd134qMevyD/v
xTWo8mVLjIJIbBUcO8Lm0alcvvkXAHYAdv+IPwq2+5VRwmHM9Ye6NLSkzbsp3GhC
Cp/mZ0xaOnQAAAGNwJuV/QAABAMARzBFAiA90a0s0Fw86i60HTH7XDtVIHnOE9sr
iosICjMRaNAzHgIhANdDshIUQaZkQM2lWPLTvmT3DKZqVMFnA5Aq425HslpPMA0G
CSqGSIb3DQEBCwUAA4IBAQBAjDmy7+UAyQqr2eYerPnwfAaSkD+3kcrbCW0Z656D
rejG3DE2yJ3q/Ao8OqZfg5hC/YpOaMvZMTwvdmLb6urWpgGdgEGVaWW+SHGrckVJ
scLqOVJ1ceXWMhMnp5QHtIhMHsBVwKPT+QM058b6oro2xamIACbAGC6eaem/TF0e
05holHHlBFPZk94PdLfB9f+nzobuWk6K+IaNihsCSgea5C31W1eWW9sE9Z9i3UXa
jbkTdgNtgRKT5HOoz4V+VPeR4uR/VBrQLrx1FsdICP6fCzG4lj1k9dJl3BLRk4xk
RJvLoyFyoRJMiqSpMcxNg7YTgK1ttUUj5BWT6rofaTxp
-----END CERTIFICATE-----
subject=CN = *.google.com
issuer=C = US, O = Google Trust Services LLC, CN = GTS CA 1C3
---
No client certificate CA names sent
Peer signing digest: SHA256
Peer signature type: ECDSA
Server Temp Key: X25519, 253 bits
---
SSL handshake has read 6813 bytes and written 406 bytes
Verification: OK
---
New, TLSv1.3, Cipher is TLS_AES_256_GCM_SHA384
Server public key is 256 bit
Secure Renegotiation IS NOT supported
Compression: NONE
Expansion: NONE
No ALPN negotiated
Early data was not sent
Verify return code: 0 (ok)
---
^C

julianz- · 2024-03-22T18:32:53Z

No I don't have anything relevant in /etc/hosts. Here's more of the error message:

openssl s_client -connect "encrypted.google.com":443 CONNECTED(00000005) depth=0 OU = "No SNI provided; please fix your client.", CN = invalid2.invalid verify error:num=18:self signed certificate

On searching for info about "No SNI provided", I see I can get correct results by adding the flag -servername to the call to openssl:

e.g.

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

gives the correct certificate result i.e. error:num = 0.

But how would I apply this fix to the test code in question? Why is it even necessary to provide the SNI on my machine but this test seems to pass on GitHub without any intervention?

julianz- · 2024-03-22T20:15:38Z

So I notice there is this line in test_set_default_verify_paths() before the call to clientSSL.do_handshake():

clientSSL.set_tlsext_host_name(b"encrypted.google.com")

This is supposed to set the SNI. So I think the error I saw with openssl s_client may have been a red herring. The actual error message (which is the first of several) I see running pytest is:

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

/Users/xxx/Library/Python/3.9/lib/python/site-packages/OpenSSL/_util.py:57: Error'

So it's saying the certificate verification failed but it's not clear why.

webknjaz · 2024-03-31T22:21:17Z

@julianz- if you add --pdb to the pytest command, are you able to inspect the exception object interactively?

Perhaps @mhils would be able to make a better advice...

julianz- · 2024-04-01T00:28:23Z

Actually I tried that but the error seems to be coming from SSL internals. After calling do_handshake() in SSL.py I can step through the python code but the error is coming from the C++ SSL library which throws an error that eventually gets translated in the command line as:

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

jlaine · 2024-06-21T19:53:01Z

src/OpenSSL/SSL.py

-except AttributeError:
-    pass
+OP_LEGACY_SERVER_CONNECT = _lib.SSL_OP_LEGACY_SERVER_CONNECT
+__all__.append("OP_LEGACY_SERVER_CONNECT")


If this is no longer conditional, wouldn't it be better to move it up with the other definitions above and include it straight into __all__? This will need a rebase since I added an int type annotation, sorry :)

webknjaz · 2024-11-07T01:08:40Z

@julianz- perhaps it's time to rebase and see if that's still an issue + maybe ask for hints from the maintainers?

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

CHANGELOG.rst Outdated Show resolved Hide resolved

CHANGELOG.rst Outdated Show resolved Hide resolved

mhils reviewed Jan 25, 2024

View reviewed changes

CHANGELOG.rst Outdated Show resolved Hide resolved