Skip to content

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

Merged
vstinner merged 1 commit into
python:mainfrom
vstinner:test_ssl3
Jun 3, 2021
Merged

bpo-43921: Cleanup test_ssl.test_wrong_cert_tls13()#26520
vstinner merged 1 commit into
python:mainfrom
vstinner:test_ssl3

Conversation

@vstinner

@vstinner vstinner commented Jun 3, 2021

Copy link
Copy Markdown
Member

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

https://bugs.python.org/issue43921

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

vstinner commented Jun 3, 2021

Copy link
Copy Markdown
Member Author

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

tiran commented Jun 3, 2021

Copy link
Copy Markdown
Member

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

@vstinner

vstinner commented Jun 3, 2021

Copy link
Copy Markdown
Member Author

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

vstinner commented Jun 3, 2021

Copy link
Copy Markdown
Member Author

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

tiran commented Jun 3, 2021

Copy link
Copy Markdown
Member

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

@miss-islington

Copy link
Copy Markdown
Contributor

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

@miss-islington

Copy link
Copy Markdown
Contributor

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

@miss-islington

Copy link
Copy Markdown
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
Copy Markdown
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

tiran commented Jun 3, 2021

Copy link
Copy Markdown
Member

@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