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

[3.5] bpo-36576: Skip test_ssl and test_asyncio tests failing with OpenSSL 1.1.1 #12694

Merged
merged 6 commits into from
Sep 7, 2019
Merged

[3.5] bpo-36576: Skip test_ssl and test_asyncio tests failing with OpenSSL 1.1.1 #12694

merged 6 commits into from
Sep 7, 2019

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Apr 5, 2019

Some test_ssl and test_asyncio are written for OpenSSL 1.0 and TLS
1.0, but fail with OpenSSL 1.1.1 and TLS 1.3.

Fixing these needs require to backport new ssl flags like
ssl.OP_NO_TLSv1_3 or ssl.OP_NO_COMPRESSION which cannot be done in a
minor 3.5.x release. Moreover, it is not really worth it: the code
works fine, issues are in the tests.

https://bugs.python.org/issue36576

@vstinner
Copy link
Member Author

vstinner commented Apr 5, 2019

I wrote a similar change for Fedora Rawhide: https://src.fedoraproject.org/rpms/python35/pull-request/23

Somehow related, I wrote a change to add OpenSSL 1.1.1 support to Python 3.4:

I may also skip failing tests on Python 3.4.

@vstinner vstinner requested a review from tiran April 5, 2019 08:35
@vstinner
Copy link
Member Author

vstinner commented Apr 5, 2019

cc @stratakis

@vstinner vstinner changed the title [3.5] bpo-26470: Skip test_ssl and test_asyncio tests failing with OpenSSL 1.1 [3.5] bpo-26470: Skip test_ssl and test_asyncio tests failing with OpenSSL 1.1.0 Apr 5, 2019
@vstinner
Copy link
Member Author

vstinner commented Apr 5, 2019

@stratakis asked me to replace "OpenSSL 1.1" with "OpenSSL 1.1.0": done.

@@ -1145,6 +1151,7 @@ def test_legacy_create_unix_server_ssl_verify_failed(self):
self.test_create_unix_server_ssl_verify_failed()

@unittest.skipIf(ssl is None, 'No ssl module')
@unittest.skipIf(IS_OPENSSL_1_1, "bpo-26470: fail on OpenSSL 1.1")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the version number 1.1 and not 1.1.0 here by accident or purpose?

Suggested change
@unittest.skipIf(IS_OPENSSL_1_1, "bpo-26470: fail on OpenSSL 1.1")
@unittest.skipIf(IS_OPENSSL_1_1, "bpo-26470: fail on OpenSSL 1.1.0")

That would make it more consistent.

Also, have we checked that it's actually 1.1.0 and not 1.1.1? Should we say 1.1 everywhere instead?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, have we checked that it's actually 1.1.0 and not 1.1.1?

No, and I'm not interested to test if it's exactly 1.1.0 or 1.1.1. I'm tired of the OpenSSL 1.1.1 mess, I consider that I already spent enough time on this topic :-)

Should we say 1.1 everywhere instead?

I fixed the test_asyncio comment to write OpenSSL 1.1.0, as I did in test_ssl.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the record, the tests pass with OpenSSL 1.1.0i.

@@ -38,6 +38,12 @@
from asyncio import test_support as support


if ssl is not None:
IS_OPENSSL_1_1 = ssl.OPENSSL_VERSION_INFO >= (1, 1, 0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Woudl a oneliner be more readable? Or less? Something like:

IS_OPENSSL_1_1 = ssl is not None and ssl.OPENSSL_VERSION_INFO >= (1, 1, 0)

Or even:

IS_OPENSSL_1_1 = ssl and ssl.OPENSSL_VERSION_INFO >= (1, 1, 0)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer to write the code on 4 lines ;-)

Copy link
Contributor

@hroncok hroncok left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good enough for 3.5.

@vstinner vstinner changed the title [3.5] bpo-26470: Skip test_ssl and test_asyncio tests failing with OpenSSL 1.1.0 [3.5] bpo-26470: Skip test_ssl and test_asyncio tests failing with OpenSSL 1.1.1 Apr 9, 2019
@vstinner vstinner changed the title [3.5] bpo-26470: Skip test_ssl and test_asyncio tests failing with OpenSSL 1.1.1 [3.5] bpo-36576: Skip test_ssl and test_asyncio tests failing with OpenSSL 1.1.1 Apr 9, 2019
Copy link
Contributor

@hroncok hroncok left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even better now! Thanks.

@vstinner
Copy link
Member Author

vstinner commented Apr 9, 2019

Oh, the NEWS entry used the old bpo number. I also fixed that.

…1.1.1

Some test_ssl and test_asyncio are written for OpenSSL 1.0 and TLS
1.0, but fail with OpenSSL 1.1.1 and TLS 1.3.

Fixing these needs require to backport new ssl flags like
ssl.OP_NO_TLSv1_3 or ssl.OP_NO_COMPRESSION which cannot be done in a
minor 3.5.x release. Moreover, it is not really worth it: the code
works fine, issues are in the tests.
@vstinner
Copy link
Member Author

vstinner commented Apr 9, 2019

Oh, my PR used IS_OPENSSL_1_1_1 but it didn't exist! I fixed that as well.

@larryhastings
Copy link
Contributor

@tiran I'm inclined to merge this patch. Do you want to review it before I merge, or should I just go ahead?

@vstinner
Copy link
Member Author

FYI Fedora now uses this patch in Python 3.5: https://src.fedoraproject.org/rpms/python35/blob/master/f/00322-test_ssl-skip-openssl111.patch

Even if right now, the package is still linked to OpenSSL 1.0: https://src.fedoraproject.org/rpms/python35/blob/master/f/python35.spec#_128

@larryhastings larryhastings merged commit 4d1c254 into python:3.5 Sep 7, 2019
@bedevere-bot
Copy link

@larryhastings: Please replace # with GH- in the commit message next time. Thanks!

@larryhastings
Copy link
Contributor

Thanks for the 3.5 love, Victor!

@vstinner vstinner deleted the skip_tests_openssl11 branch January 30, 2020 12:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests Tests in the Lib/test dir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants