Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix problem caused by SSL_WANT_READ or SSL_WANT_WRITE errors. #1242

Closed
wants to merge 7 commits into from

Conversation

julianz-
Copy link
Contributor

@julianz- julianz- commented Aug 18, 2023

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 adding 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.

…TE 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.
@reaperhulk
Copy link
Member

Amazing to see how long this has been in CPython without ever coming up here!. Unfortunately we removed SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER from the bindings in cryptography in 40.0.0 (pyca/cryptography@895de04) so we'll need to re-add it there, do a release, and then release a new pyOpenSSL with version bounds. Would you be interested in sending a PR to cryptography to add this constant again?

@mhils
Copy link
Member

mhils commented Aug 18, 2023

Thanks, interesting PR indeed! I'll point out that this is a bit of a double-edged sword, we're essentially disabling an OpenSSL assertion that retries are done correctly ("This is not the default to avoid the misconception that non-blocking SSL_write() behaves like non-blocking write()."1). The entire "buffers in python can change location" argument seems to stem from repeated buffer.getvalue() calls, which should probably be cached in the client app...

Anyhow - it's a tradeoff between catching one of two API misuses, and even if I don't fully agree with all arguments it's probably reasonable to follow CPython here. :)

Footnotes

  1. https://www.openssl.org/docs/man1.0.2/man3/SSL_CTX_set_mode.html

@mhils
Copy link
Member

mhils commented Aug 18, 2023

Funnily enough, https://bugs.python.org/issue8240#msg101986 (2010-03-31) mentions pyOpenSSL as precedent for enabling this by default in CPython. "Some initialization that's required to operate smoothly in Python". 😄

@julianz-
Copy link
Contributor Author

Amazing to see how long this has been in CPython without ever coming up here!. Unfortunately we removed SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER from the bindings in cryptography in 40.0.0 (pyca/cryptography@895de04) so we'll need to re-add it there, do a release, and then release a new pyOpenSSL with version bounds. Would you be interested in sending a PR to cryptography to add this constant again?

Thank you for your comment! I raised this issue quite some time ago but didn't quite know the best way to go about dealing with it. Will send a PR in cryptography as you suggest.

julianz- added a commit to julianz-/cryptography that referenced this pull request Aug 18, 2023
This constant was removed in pyca@895de04 but it is still needed to deal with an issue in PyOpenSSL described here cherrypy/cheroot#245 and PR pyca/pyopenssl#1242.
alex pushed a commit to pyca/cryptography that referenced this pull request Aug 18, 2023
This constant was removed in 895de04 but it is still needed to deal with an issue in PyOpenSSL described here cherrypy/cheroot#245 and PR pyca/pyopenssl#1242.
…NG_WRITE_BUFFER

In order to fix issue described here cherrypy/cheroot#245, we need to use this constant that was removed from https://github.com/pyca/cryptography but now restored
@julianz-
Copy link
Contributor Author

julianz- commented Aug 18, 2023

I fixed the cryptography issue and a few issues raised by flake8/black but I still can't seem to get my PR to pass all tests. Any advice would be appreciated.

@reaperhulk
Copy link
Member

This PR runs into the flake8 issue we have in main + the part where it depends on an unreleased cryptography (whose version is likely 42.0.0 but we don't have a release date for). We won't be able to merge this until we have a release of cryptography, but we can revisit as soon as that ships. At that point we can update the minimums, add changelog entries, and get this merged.

@julianz-
Copy link
Contributor Author

This PR runs into the flake8 issue we have in main + the part where it depends on an unreleased cryptography (whose version is likely 42.0.0 but we don't have a release date for). We won't be able to merge this until we have a release of cryptography, but we can revisit as soon as that ships. At that point we can update the minimums, add changelog entries, and get this merged.

Thanks for the information. I was able to make the changes in my fork in order to pass those flake8 issues but I see we have to await a new release of cryptography now even though the change I requested there was accepted.

@webknjaz
Copy link

@julianz- There were 5 releases of cryptography since your last comment. The last one happened 16 hours ago. Time to rebase and see what the CI has to say?

Need v42.0.0 or later of Cryptography as this restored the SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER constant which is required for possible fix per cherrypy/cheroot#245
@webknjaz
Copy link

@julianz- maybe rebase, still? So that it picks up the RTD fixes on main.

@webknjaz
Copy link

@julianz- by the way, I have a tip for you regarding sending PRs from forks: https://hynek.me/articles/pull-requests-branch/

@julianz-
Copy link
Contributor Author

@webknjaz

by the way, I have a tip for you regarding sending PRs from forks: https://hynek.me/articles/pull-requests-branch/

I followed your advice and started a new branch based on the current code and then did a new pull request from that branch. So I guess I should cancel or delete this pull-request? Even so, the new pull-request failed to pass the coverage test for reasons that are currently mysterious to me. Using Cryptography v42 maybe disturbed the coverage?

@mhils
Copy link
Member

mhils commented Jan 25, 2024

Closing this in favor of #1287 :)

@mhils mhils closed this Jan 25, 2024
@webknjaz
Copy link

Even so, the new pull-request failed to pass the coverage test for reasons that are currently mysterious to me. Using Cryptography v42 maybe disturbed the coverage?

Codecov is drunk. The uploads are often flaky, resulting in incomplete coverage showing up in that web service. That's probably what you were seeing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants