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

Switch default ssl protocol to PROTOCOL_TLS_CLIENT and improve tests #207

Merged
merged 2 commits into from
Apr 15, 2024

Conversation

ml31415
Copy link
Collaborator

@ml31415 ml31415 commented Apr 15, 2024

TLDR:

  • Make sure, no errors slip unnoticed in spawned greenlets
  • Mute expected error output in SSL tests
  • Set PROTOCOL_TLS_CLIENT by default instead of outdated gevent default PROTOCOL_SSLv23
  • Move common tests tooling to conftest.py
  • Isolate http.client patching to the respective tests

In our current test suite, there are lots of cases when a local server is spun up in a separate greenlet. Errors in these greenlets so far only threw output to std.err, but didn't raise an actual error. This behavior is dangerous, as it can easily hide errors and pretend a passing test suite.

One test in test_ssl.py had confusing delayed output to std.err. This made test runs with pytest -v -s look like some random tests were erroring. This is fixed now. and the output to std.err is caught cleanly.

The current gevent.ssl default protocol version for wrapping a SSL socket is still ssl.PROTOCOL_SSLv23. This is outdated since python 3.10 and was replaced with ssl.PROTOCOL_TLS_CLIENT. This PR makes use of the new protocol, while the default in gevent unfortunately is still unchanged. In short: This makes sure, our SSL sockets don't negotiate an outdated and possibly less secure connection.

Some common code, which was present multiple times in different test modules, was moved to conftest.py. Makes maintenance easier.

The tests now also don't monkey patch http.client in general, but only for the tests of httplib2 and urllib.request, that depend on that patching. As the monkey patching is only required for a small subset of features, this makes sure, that the tests run in the environment, in which they'd be commonly used.

Make sure, no errors slip unnoticed in spawned greenlets

Mute expected error output in SSL tests

Isolate http.client patching to the respective tests

Set PROTOCOL_TLS_CLIENT by default instead of outdated gevent default PROTOCOL_SSLv23
@cyberw
Copy link
Collaborator

cyberw commented Apr 15, 2024

I wont pretend to understand the implications of this change :) LGTM

Maybe have a look at the commit messages and PR title though (so they are focused on the actual code changes instead of the changes to tests)

@ml31415
Copy link
Collaborator Author

ml31415 commented Apr 15, 2024

Yeah, thanks for the hint. I was clearly a bit too short and cryptic with the description and the rationale of the changes.

@ml31415 ml31415 merged commit 12ea4b0 into master Apr 15, 2024
12 checks passed
@ml31415
Copy link
Collaborator Author

ml31415 commented Apr 15, 2024

For the SSL deprecations, see: https://docs.python.org/3/library/ssl.html#ssl.PROTOCOL_SSLv23

@cyberw
Copy link
Collaborator

cyberw commented Apr 15, 2024

Maybe the PR title (which will be included in the release notes) could be something like "Switch default ssl protocol to PROTOCOL_TLS_CLIENT and improve tests"?

@ml31415 ml31415 changed the title Testing and SSL improvements Switch default ssl protocol to PROTOCOL_TLS_CLIENT and improve tests Apr 15, 2024
@ml31415 ml31415 deleted the ssl_test_improvements branch April 15, 2024 11:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants