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

bpo-43921: Cleanup test_ssl.test_wrong_cert_tls13() #26520

Merged
merged 1 commit into from
Jun 3, 2021
Merged

bpo-43921: Cleanup test_ssl.test_wrong_cert_tls13() #26520

merged 1 commit into from
Jun 3, 2021

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Jun 3, 2021

Don't catch OSError, and check the SSLError message.

https://bugs.python.org/issue43921

Don't catch OSError, and check the SSLError message.
@vstinner
Copy link
Member Author

vstinner commented Jun 3, 2021

I don't see why a SSLSocket would raise an OSError on read()/write().

On ECONNRESET error, read() raises an SSLEOFError exception: see https://bugs.python.org/issue43921#msg394967

cc @tiran

@tiran
Copy link
Member

tiran commented Jun 3, 2021

It's based on old code from Antoine, see test_wrong_cert_tls12 and commit a6a4dc8.

@vstinner
Copy link
Member Author

vstinner commented Jun 3, 2021

It's based on old code from Antoine, see test_wrong_cert_tls12 and commit a6a4dc8.

I see. For test_wrong_cert_tls12(), SSLError or OSError is expected on connect(), not on read/write. There is a comment explaining the OSError:

# Expect either an SSL error about the server rejecting
# the connection, or a low-level connection reset (which
# sometimes happens on Windows)
s.connect((HOST, server.port))

test_wrong_cert_tls13() is different because TLS 1.3 is different. The test doesn't expect any error on connect(), only on read() or write(). It also explained in a comment:

# TLS 1.3 perform client cert exchange after handshake

In short, test_wrong_cert_tls12() looks correct, and I now understand how test_wrong_cert_tls13() inherited except OSError:. So I'm not convinced that it's ok to remove it :-)

@vstinner vstinner merged commit 5c2191d into python:main Jun 3, 2021
@vstinner vstinner deleted the test_ssl3 branch June 3, 2021 20:12
@vstinner
Copy link
Member Author

vstinner commented Jun 3, 2021

This change makes the test more strict (check the exact error message). I'm not sure that it works well on all platforms and all supported OpenSSL versions. I prefer to not backport the change to 3.10. At least, not now.

@tiran
Copy link
Member

tiran commented Jun 3, 2021

Either keep 3.9 to 3.11 tests in sync or don't merge changes.

@tiran tiran added needs backport to 3.9 only security fixes needs backport to 3.10 only security fixes labels Jun 3, 2021
@miss-islington
Copy link
Contributor

Thanks @vstinner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.10.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

Thanks @vstinner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.9.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

Sorry @vstinner, I had trouble checking out the 3.10 backport branch.
Please backport using cherry_picker on command line.
cherry_picker 5c2191df9a21a3b3d49dd0711b8d2b92591ce82b 3.10

@miss-islington
Copy link
Contributor

Sorry, @vstinner, I could not cleanly backport this to 3.9 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 5c2191df9a21a3b3d49dd0711b8d2b92591ce82b 3.9

@tiran
Copy link
Member

tiran commented Jun 3, 2021

@vstinner please backport changes to 3.10 and 3.9. Tests should be kept in sync.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip news tests Tests in the Lib/test dir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants