bpo-35926: Add support for OpenSSL 1.1.1b on Windows #11779

paulmon · 2019-02-07T02:55:44Z

After a little more testing I discovered that these changes don't quite pass all of the tests yet.
I will be looking into these failures tomorrow

amd64 retail - 1 failure
amd64 debug - 0 failures
win32 retail - 1 failures
win32 debug - access violation halts test

test_parse_cert_CVE_2019_5010 only fails win32 debug (access violation)
works for amd64 debug/release and win32 release

test_load_default_certs_env_windows fails on win32 and amd64 retail. skipped on debug

https://bugs.python.org/issue35926

the-knights-who-say-ni · 2019-02-07T02:55:46Z

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

You can check yourself to see if the CLA has been received.

Thanks again for your contribution, we look forward to reviewing it!

paulmon · 2019-02-07T18:23:12Z

PCbuild/openssl.vcxproj

+    <ConfigurationType>Makefile</ConfigurationType>
+    <Bitness>64</Bitness>
+    <ArchName>amd64</ArchName>
+    <OpenSSLPlatform>VC-WIN64A</OpenSSLPlatform>


appveyor.yml uses VC-WIN64A-masm for this build. Should this be updated?

Do you mean appveyor in the OpenSSL repository? If so, yeah, we should match them.

yes that's what I meant

Changing to VC-WIN64A-masm breaks the cpython build with a message that's not helpful:
Error: The operation could not be completed. Unspecified error

If I'm reading the configure files correctly then "VC-WIN64A" is the 64-bit equivalent of "VC-WIN32 no-asm" and it works with the cpython build.

This file shouldn't be being used in the CPython build - it's used to build OpenSSL separately and then we dynamically link to it (I might consider moving it to Tools to better reflect this...). Maybe I'll have to try it and see what happens.

It's not used in the regular build but I needed to build openssl with the patch the prepare_ssl.bat applies before I could test changing to openssl 1.1.1a. I assumed that this script is used to build the binaries for cpython-bin-deps based on the comments in PCBuild/readme.txt

Actually, this file isn't used at all for the official builds I've been doing (example). And it doesn't quite match how I've been building either... I'll take a look and see if I can get the build done.

paulmon · 2019-02-07T20:08:19Z

The access violation was fixed by a clean build.

The other failure looks like the value for x509 was incremented in the certificate store compared to the keycert.pem file that is checked in. I'm still trying to learn what this means.

176 != 177,
AssertionError: {'x509': 176, 'crl': 0, 'x509_ca': 137} != {'x509': 177, 'crl': 0, 'x509_ca': 137}

paulmon · 2019-02-07T21:45:10Z

test_load_default_certs_env_windows is failing in my test build because after calling _wputenv(…) from EnvironmentVarGaurd() in python, when openssl calls getenv the environment variable isn't set so the key that was added to the path with the environment variables isn't counted... still trying to figure out why this happens. Could python and openssl be linking to different CRTs even though they are both retail?

This ended up being caused by using a debug openssl.dll with a release build of python.

Lib/test/test_ssl.py

zooba · 2019-02-08T16:59:22Z

PCbuild/openssl.vcxproj

+    <ConfigurationType>Makefile</ConfigurationType>
+    <Bitness>64</Bitness>
+    <ArchName>amd64</ArchName>
+    <OpenSSLPlatform>VC-WIN64A</OpenSSLPlatform>


This file shouldn't be being used in the CPython build - it's used to build OpenSSL separately and then we dynamically link to it (I might consider moving it to Tools to better reflect this...). Maybe I'll have to try it and see what happens.

paulmon · 2019-02-08T18:35:52Z

I ended up creating a new clone, rebuilding openssl with the current version of prepare_ssl, and building cpython clean. When I run the tests for x86, and amd64, debug and retail for each they all currently pass.

tiran

Thanks for working on this. For some unknown reason, Windows behaves slightly different than Linux. I wonder what's going on.

#10031 adds some debug helpers to analyse the protocol. I'll try to land the PR first, then you can use it to diagnose the handshake.

Lib/test/test_ssl.py

tiran · 2019-02-09T12:29:15Z

Lib/test/test_ssl.py

-                    self.close()
-                    self.running = False
+                except OSError as err:
+                    if 'peer did not return a certificate' in err.args[1]:


I wonder why I haven't run into this issue on Linux.

The error is coming from Winsock. Could there be a difference in the way Winsock handles certs and the way linux sockets handle certs that is causing this difference?

Most of the tests that are failing are failing in the same way as Linux. I can tell because there a comments and expected exceptions in multiple places that say something like "sometimes connections fail with TLS 1.3"

The tests are opening a socket doing a handshake and the disconnecting the client before the session tickets are finished sending which results in various disconnect errors. A real client would send something before closing the session, which appears to allow the session tickets to get sent. In my mind these are bad tests.

There are also these issues in the openssl issues about the session ticket protocol: openssl/openssl#7948, openssl/openssl#7967, python-trio/trio#819 which don't really seem resolved.

What is needed to move this PR forward?

Looking at the first of those issues, it seems like we should fix our tests to be negotiating the close properly.

@tiran Why the OSError case is different on Linux I'm not sure. On windows the OSError I was trying to handle was SSLError exception (based on OSError). There is a race condition between the server getting the PEER_DID_NOT_RETURN_A_CERTIFICATE error and the client receiving the "tvs13 alert ..." error on Windows. One of them always arrives but the protocol doesn't look like it guarantees enough synchronicity to force one of them to be always be first.

@zooba The first 3 tests failures were caused by a bug in openssl so I have removed the try/excepts.
I have submitted patches for openssl
openssl/openssl#8590
python/cpython-source-deps#13

zooba · 2019-02-10T02:08:44Z

I queued up a build on pipelines here with the built binaries. It looked good apart from one thing that I'm about to comment on.

zooba · 2019-02-10T02:13:53Z

Correction - not going to point it out on the file because apparently you haven't touched the file?

PCbuild/openssl.props needs updating. In particular, x64 is no longer added to the filenames for the 64-bit build (though -arm and -arm64 are added for those builds).

paulmon · 2019-02-11T19:10:00Z

The -x64 is dropped when you change the openssl build type from VC-WIN64A to VC-WIN64A-masm. The props file seems to be the source of my mysterious build failure when I was trying out the VC-WIN64A-masm build.

zooba · 2019-02-11T19:31:59Z

I'm not seeing why Bedevere thinks the NEWS file is incorrectly formatted - it looks fine to me? I renamed the PR to see if that helps, but apparently not (or maybe it needs a new commit to reevaluate?)

Maybe try deleting and recreating the NEWS entry? Also specify in the text that this only updates Windows - @ned-deily will add his own entry for macOS.

ned-deily · 2019-02-11T20:20:24Z

I also don't see anything wrong with the news entry and, more importantly, neither does blurb when I run it locally. I don't know what's going on on the bot side. If it persists, we may need help from @Mariatta or @brettcannon.

Mariatta · 2019-02-11T20:29:35Z

Looks fine, I'll try adding the skip news and removing it, in case it retrigger the check.

Mariatta · 2019-02-11T20:43:39Z

The news check failed because it is expecting the news entry to be more than 45 characters, so please re-write the news entry.

Mariatta · 2019-02-11T20:47:53Z

Edit: the actual news entry has to be at least 30 characters.

ned-deily · 2019-02-11T21:00:10Z

@Mariatta Can you say what is expecting the news entry to be that long and why? AFAIK, neither blurb nor the docs build imposes a requirement.

Mariatta · 2019-02-11T21:05:48Z

@ned-deily There was discussion here: python/bedevere#127

ned-deily · 2019-02-11T21:20:03Z

I guess we now have a counter-example. Personally, I don’t see a need to enforce a limit in the bot; in the end the release manager will review and edit the news entries anyway prior to release. OTOH I suppose the item could be padded.

Mariatta · 2019-02-11T21:23:54Z

🤔 add "Patch by Paul Monson."? 😅

I'm fine to tweak this to "as long as file not empty" if that's what we want.

ned-deily · 2019-02-11T21:28:33Z

That’s one solution!

I think the fact that we’ve all spent so much time on tracking this down and discussing it says to me that having the arbitrary limit is more trouble than it is worth.

zooba · 2019-04-29T22:03:19Z

Still hoping for @tiran to take a look at the SSL test changes, but since I'll be in the same room as him in a couple of days I can "negotiate" more strongly.

I think we want the OpenSSL update for sure though, so getting this done ought to be easy enough.

zooba · 2019-04-29T23:03:17Z

@paulmon There's a new OpenSSL build available. Will that remove the need for the changes to the tests (i.e. we're throwing the right exception now)?

paulmon · 2019-04-29T23:27:00Z

That patch gets rid of the exceptions that were removed in this commit: 016a485. I looked at the other exceptions with wireshark and didn't spot a way to make them match what happens on linux. I'm taking another look at the changes to verify whether they are fixed by the new build.

paulmon · 2019-04-30T23:53:32Z

Lib/test/test_ssl.py

+                        if self.server.chatty and support.verbose:
+                            sys.stdout.write(err.args[1])
+                        # test_pha_required_nocert is expecting this exception
+                        raise ssl.SSLError('tlsv13 alert certificate required')


It seems that sockets are blocking by default on Linux, and non-blocking on Windows. This could cause this difference, maybe. When I step through the socket creation code the socket is created on this line on Windows:

fd = WSASocketW(family, type, proto, NULL, 0, WSA_FLAG_OVERLAPPED | WSA_FLAG_NO_HANDLE_INHERIT);

WSA_FLAG_OVERLAPPED specifies a non-blocking socket.

I think the remaining errors are caused when the server side fails first, but would fail on the client first if the sockets were blocking

zooba · 2019-05-01T15:47:16Z

Okay, we may have to hold this for a bit longer.

I'll see if I can get some people who know this area to look it over at PyCon.

paulmon · 2019-05-01T19:42:12Z

I'll see if I can get some people who know this area to look it over at PyCon.

I think that would be really helpful. Thanks!

zooba · 2019-05-14T21:06:43Z

I've run the test suite a few times without issues, and I'm doing a custom buildbot run right now. If that comes up clear, I see no reason to keep from merging this.

zooba · 2019-05-14T22:29:39Z

@tiran Any last comments or concerns?

.travis.yml

zooba · 2019-05-15T20:53:35Z

Just noticed one more change to the Travis CI configuration. Which seems to work okay, but isn't part of enabling OpenSSL 1.1.1b on Windows so we'll let Christian do it when he's ready.

Co-Authored-By: Steve Dower <steve.dower@microsoft.com>

paulmon · 2019-05-15T22:17:33Z

@zooba Makes sense to me. Merged your suggested change.

miss-islington · 2019-05-15T22:43:38Z

Thanks @paulmon for the PR, and @zooba for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

miss-islington · 2019-05-15T22:43:53Z

Sorry, @paulmon and @zooba, I could not cleanly backport this to 3.7 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker fb7e7505ed1337bf40fa7b8b68317d1e86675a86 3.7

bedevere-bot · 2019-05-15T22:47:15Z

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

paulmon requested a review from a team as a code owner Feb 7, 2019

the-knights-who-say-ni added the CLA not signed label Feb 7, 2019

bedevere-bot added the awaiting review label Feb 7, 2019

paulmon commented Feb 7, 2019

View changes

zooba reviewed Feb 8, 2019

View changes

tiran reviewed Feb 9, 2019

View changes

zooba changed the title ~~bpo-35926code and test changes for OpenSSL 1.1.1a for Windows~~ bpo-35926: Add support for OpenSSL 1.1.1a on Windows Feb 11, 2019

Mariatta added skip news and removed skip news labels Feb 11, 2019

the-knights-who-say-ni added CLA signed and removed CLA not signed labels Feb 11, 2019

paulmon mentioned this pull request Apr 29, 2019

assign Windows arm32 worker python/buildmaster-config#89

Merged

paulmon added 2 commits Apr 29, 2019

merge master

d864d54

remove one more exception handler

12c5be2

paulmon commented Apr 30, 2019

View changes

ConnectionAbortedError is still happening

a117d82

zooba mentioned this pull request May 14, 2019

bpo-35926: Update Windows build to OpenSSL 1.1.1b #13323

Closed

zooba reviewed May 15, 2019

View changes

.travis.yml Outdated Show resolved Hide resolved

Update .travis.yml

f892ad7

Co-Authored-By: Steve Dower <steve.dower@microsoft.com>

zooba merged commit fb7e750 into python:master May 15, 2019

bedevere-bot removed the awaiting changes label May 15, 2019

zooba added needs backport to 3.7 OS-windows labels May 15, 2019

miss-islington assigned zooba May 15, 2019

zooba pushed a commit to zooba/cpython that referenced this pull request May 15, 2019

bpo-35926: Add support for OpenSSL 1.1.1b on Windows (pythonGH-11779)

26db00f

bedevere-bot removed the needs backport to 3.7 label May 15, 2019

zooba added a commit that referenced this pull request May 16, 2019

bpo-35926: Add support for OpenSSL 1.1.1b on Windows (GH-11779)

aa73841

paulmon deleted the openssl-111a branch Jun 19, 2019

jjhelmus mentioned this pull request Oct 31, 2019

python v3.8.0 conda-forge/python-feedstock#274

Merged

3 tasks

bpo-35926: Add support for OpenSSL 1.1.1b on Windows #11779

bpo-35926: Add support for OpenSSL 1.1.1b on Windows #11779

Conversation

paulmon commented Feb 7, 2019 • edited by bedevere-bot

the-knights-who-say-ni commented Feb 7, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulmon commented Feb 7, 2019

paulmon commented Feb 7, 2019 • edited

Choose a reason for hiding this comment

paulmon commented Feb 8, 2019

tiran left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulmon Mar 26, 2019 • edited

Choose a reason for hiding this comment

zooba commented Feb 10, 2019

zooba commented Feb 10, 2019

paulmon commented Feb 11, 2019

zooba commented Feb 11, 2019

ned-deily commented Feb 11, 2019

Mariatta commented Feb 11, 2019

Mariatta commented Feb 11, 2019

Mariatta commented Feb 11, 2019

ned-deily commented Feb 11, 2019 • edited

Mariatta commented Feb 11, 2019

ned-deily commented Feb 11, 2019

Mariatta commented Feb 11, 2019

ned-deily commented Feb 11, 2019

zooba commented Apr 29, 2019

zooba commented Apr 29, 2019

paulmon commented Apr 29, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zooba commented May 1, 2019

paulmon commented May 1, 2019

zooba commented May 14, 2019

zooba commented May 14, 2019

zooba commented May 15, 2019

paulmon commented May 15, 2019

miss-islington commented May 15, 2019

miss-islington commented May 15, 2019

bedevere-bot commented May 15, 2019

paulmon commented Feb 7, 2019 •

edited by bedevere-bot

paulmon commented Feb 7, 2019 •

edited

paulmon Mar 26, 2019 •

edited

ned-deily commented Feb 11, 2019 •

edited

paulmon commented Apr 29, 2019 •

edited