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.

Sign up for GitHub

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

Jump to bottom

bpo-35031: Fix test_start_tls_server_1 on FreeBSD buildbots #10011

Merged
merged 3 commits into from
Oct 29, 2018
Merged

bpo-35031: Fix test_start_tls_server_1 on FreeBSD buildbots #10011

merged 3 commits into from
Oct 29, 2018

Conversation

pablogsal
Copy link
Member

@pablogsal pablogsal commented Oct 20, 2018 •
edited by bedevere-bot
Loading

@pablogsal pablogsal requested a review from 1st1 October 20, 2018 17:00
@bedevere-bot bedevere-bot added tests Tests in the Lib/test dir awaiting merge labels Oct 20, 2018
@pablogsal
Copy link
Member Author

Tested on the buildbot itself:

CURRENT-amd64% uname -a
FreeBSD CURRENT-amd64 12.0-ALPHA10 FreeBSD 12.0-ALPHA10 r339405 GENERIC-NODEBUG  amd64
CURRENT-amd64% ./python -m test test_asyncio -m test_start_tls_server_1 -v
== CPython 3.8.0a0 (tags/v3.7.0a4-747-g35230d08e0:35230d08e0, Oct 21 2018, 03:49:42) [Clang 6.0.1 (tags/RELEASE_601/final 335540)]
== FreeBSD-12.0-ALPHA10-amd64-64bit-ELF little-endian
== cwd: /usr/home/pablo/cpython/build/test_python_32337
== CPU count: 4
== encodings: locale=UTF-8, FS=utf-8
Run tests sequentially
0:00:00 load avg: 3.63 [1/1] test_asyncio
test_start_tls_server_1 (test.test_asyncio.test_sslproto.ProactorStartTLSTests) ... skipped 'Windows only'
test_start_tls_server_1 (test.test_asyncio.test_sslproto.SelectorStartTLSTests) ... ok

----------------------------------------------------------------------
Ran 2 tests in 0.424s

OK (skipped=1)
1 test OK.

Total duration: 1 sec
Tests result: SUCCESS
CURRENT-amd64%

@pablogsal pablogsal requested a review from asvetlov October 20, 2018 17:01
@pablogsal pablogsal requested a review from vstinner October 25, 2018 22:08
@pablogsal
Copy link
Member Author

Example of the current failure:

https://buildbot.python.org/all/#/builders/173/builds/57

@vstinner vstinner requested a review from tiran October 25, 2018 23:47
@pablogsal pablogsal mentioned this pull request Oct 26, 2018
Merged
vstinner
vstinner approved these changes Oct 29, 2018
View reviewed changes
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM.

Honestly, I'm not a TLS expert, but this bug is annoying and makes the buildbot fail randomly. So I suggest to apply it as soon as possible :-)

If it's wrong, it can be fixed later, but at least the buildbot will become useful again and stop to spam buildbot-status and random PRs.

1st1
1st1 reviewed Oct 29, 2018
View reviewed changes
Lib/test/test_asyncio/test_sslproto.py Outdated
@@ -429,6 +429,7 @@ def test_start_tls_server_1(self):

server_context = test_utils.simple_server_sslcontext()
client_context = test_utils.simple_client_sslcontext()
client_context.options |= ssl.OP_NO_TLSv1_3
Copy link
Member

Choose a reason for hiding this comment

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

Can we add a comment explaining this line?

Copy link
Member Author

@pablogsal pablogsal Oct 29, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Done in 93e66f6

Copy link
Member

Choose a reason for hiding this comment

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

Can we please restrict this to FreeBSD?

@1st1 In the long run, we should have parametrized tests to test TLS 1.2 and TLS 1.3 behavior. The protocols behave differently on mulitple accounts. For client cert auth, we even need to have TLS 1.3 with and without PHA.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in f777fa5

@pablogsal pablogsal self-assigned this Oct 29, 2018
tiran
tiran reviewed Oct 29, 2018
View reviewed changes
Copy link
Member

@tiran tiran left a comment

Choose a reason for hiding this comment

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

Please limit ssl.OP_NO_TLSv1_3 to FreeBSD for now.

Lib/test/test_asyncio/test_sslproto.py Outdated
@@ -429,6 +429,7 @@ def test_start_tls_server_1(self):

server_context = test_utils.simple_server_sslcontext()
client_context = test_utils.simple_client_sslcontext()
client_context.options |= ssl.OP_NO_TLSv1_3
Copy link
Member

Choose a reason for hiding this comment

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

Can we please restrict this to FreeBSD?

@1st1 In the long run, we should have parametrized tests to test TLS 1.2 and TLS 1.3 behavior. The protocols behave differently on mulitple accounts. For client cert auth, we even need to have TLS 1.3 with and without PHA.

@pablogsal
Copy link
Member Author

pablogsal commented Oct 29, 2018 •
edited
Loading

@tiran I restricted the fix for FreeBSD in commit f777fa5.

@pablogsal pablogsal merged commit f6a47f3 into python:master Oct 29, 2018
@pablogsal pablogsal deleted the bpo35031 branch October 29, 2018 20:47
@miss-islington
Copy link
Contributor

Thanks @pablogsal for the PR 🌮🎉.. I'm working now to backport this PR to: 3.6.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

@miss-islington
Copy link
Contributor

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

@miss-islington
Copy link
Contributor

Sorry, @pablogsal, I could not cleanly backport this to 3.6 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker f6a47f3e316cede2a07a1f74a509f6d80ab8fef0 3.6

@bedevere-bot
Copy link

GH-10496 is a backport of this pull request to the 3.7 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Nov 13, 2018
@pablogsal @miss-islington
bpo-35031: Fix test_start_tls_server_1 on FreeBSD buildbots (pythonGH…
7aef645 
…-10011)

Some FreeBSD buildbots fail to run this test as the eof was not being received by the server if the size is not big enough. This behaviour only appears if the client is using TLS1.3.
(cherry picked from commit f6a47f3)

Co-authored-by: Pablo Galindo <Pablogsal@gmail.com>
@pablogsal
Copy link
Member Author

Backporting after checking that it works on the buildbots.

@pablogsal
Copy link
Member Author

Apparently, the issue in 3.6 is different. It happens on test_ssl. Example:

https://buildbot.python.org/all/#/builders/172/builds/86/steps/4/logs/stdio

miss-islington added a commit that referenced this pull request Nov 13, 2018
@miss-islington @pablogsal
bpo-35031: Fix test_start_tls_server_1 on FreeBSD buildbots (GH-10011)
3f9fe53 
Some FreeBSD buildbots fail to run this test as the eof was not being received by the server if the size is not big enough. This behaviour only appears if the client is using TLS1.3.
(cherry picked from commit f6a47f3)

Co-authored-by: Pablo Galindo <Pablogsal@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@1st1 1st1 1st1 left review comments

@tiran tiran tiran left review comments

@vstinner vstinner vstinner approved these changes

@asvetlov asvetlov Awaiting requested review from asvetlov

Assignees

@pablogsal pablogsal

Labels
skip news tests Tests in the Lib/test dir
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@pablogsal @miss-islington @bedevere-bot @vstinner @1st1 @tiran @the-knights-who-say-ni