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-35926: Add support for OpenSSL 1.1.1b on Windows #11779

Open
wants to merge 13 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@paulmon
Copy link
Contributor

paulmon commented Feb 7, 2019

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

@paulmon paulmon requested a review from python/windows-team as a code owner Feb 7, 2019

@the-knights-who-say-ni

This comment has been minimized.

Copy link

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

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!

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

This comment has been minimized.

@paulmon

paulmon Feb 7, 2019

Author Contributor

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

This comment has been minimized.

@zooba

zooba Feb 7, 2019

Member

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

This comment has been minimized.

@paulmon

paulmon Feb 8, 2019

Author Contributor

yes that's what I meant

This comment has been minimized.

@paulmon

paulmon Feb 8, 2019

Author Contributor

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 comment has been minimized.

@zooba

zooba Feb 8, 2019

Member

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.

This comment has been minimized.

@paulmon

paulmon Feb 8, 2019

Author Contributor

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

This comment has been minimized.

@zooba

zooba Feb 8, 2019

Member

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

This comment has been minimized.

Copy link
Contributor Author

paulmon commented Feb 7, 2019

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

This comment has been minimized.

Copy link
Contributor Author

paulmon commented Feb 7, 2019

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.

@@ -2215,7 +2218,7 @@ def wrap_conn(self):
if support.verbose and self.server.chatty:
sys.stdout.write(" client cert is " + pprint.pformat(cert) + "\n")
cert_binary = self.sslconn.getpeercert(True)
if support.verbose and self.server.chatty:
if support.verbose and self.server.chatty and cert_binary != None:

This comment has been minimized.

@zooba

zooba Feb 8, 2019

Member

@tiran This would have been needed due to the recent CVE fix, right? Why didn't we have to put it in already? Or did we not add tests for this case.

This comment has been minimized.

@tiran

tiran Feb 9, 2019

Member

Which CVE are you referring to? The CRL DP NULL pointer CVE? That shouldn't be related here.

With TLS 1.3 getpeercert returns None when the client has not send a client auth cert yet. This should only occur when the server-side has enable post handshake auth.

This comment has been minimized.

@paulmon

paulmon Feb 12, 2019

Author Contributor

I turned chatty on temporarily while testing and hit this code path which isn't normally hit. I don't think it's new behavior

@@ -4251,7 +4258,7 @@ def test_pha_required_nocert(self):
server_context.verify_mode = ssl.CERT_REQUIRED
client_context.post_handshake_auth = True

server = ThreadedEchoServer(context=server_context, chatty=False)
server = ThreadedEchoServer(context=server_context, chatty=True)

This comment has been minimized.

@zooba

zooba Feb 8, 2019

Member

Remember to reset this default once all the tests are fixed.

This comment has been minimized.

@paulmon

paulmon Feb 8, 2019

Author Contributor

This is the other half of the 'peer did not return a certificate' error change above.
The 'tlsv13 alert certificate required' below was not being raised because of the error in server.
Looking at the other code in this test I don't know if I changed the test correctly but I think it illustrates what I'm seeing.

This comment has been minimized.

@paulmon

paulmon Feb 8, 2019

Author Contributor

I reworked this so that chatty=False.
I think the new commit makes more sense.

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

This comment has been minimized.

@zooba

zooba Feb 8, 2019

Member

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

This comment has been minimized.

Copy link
Contributor Author

paulmon commented Feb 8, 2019

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

tiran left a comment

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.

@@ -2215,7 +2218,7 @@ def wrap_conn(self):
if support.verbose and self.server.chatty:
sys.stdout.write(" client cert is " + pprint.pformat(cert) + "\n")
cert_binary = self.sslconn.getpeercert(True)
if support.verbose and self.server.chatty:
if support.verbose and self.server.chatty and cert_binary != None:

This comment has been minimized.

@tiran

tiran Feb 9, 2019

Member

Which CVE are you referring to? The CRL DP NULL pointer CVE? That shouldn't be related here.

With TLS 1.3 getpeercert returns None when the client has not send a client auth cert yet. This should only occur when the server-side has enable post handshake auth.

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

This comment has been minimized.

@tiran

tiran Feb 9, 2019

Member

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

This comment has been minimized.

@paulmon

paulmon Feb 12, 2019

Author Contributor

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?

This comment has been minimized.

@paulmon

paulmon Mar 5, 2019

Author Contributor

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?

@zooba

This comment has been minimized.

Copy link
Member

zooba commented Feb 10, 2019

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

This comment has been minimized.

Copy link
Member

zooba commented Feb 10, 2019

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

This comment has been minimized.

Copy link
Contributor Author

paulmon commented Feb 11, 2019

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 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

@zooba

This comment has been minimized.

Copy link
Member

zooba commented Feb 11, 2019

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

This comment has been minimized.

Copy link
Member

ned-deily commented Feb 11, 2019

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

This comment has been minimized.

Copy link
Member

Mariatta commented Feb 11, 2019

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

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

@Mariatta

This comment has been minimized.

Copy link
Member

Mariatta commented Feb 11, 2019

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

This comment has been minimized.

Copy link
Member

Mariatta commented Feb 11, 2019

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

@ned-deily

This comment has been minimized.

Copy link
Member

ned-deily commented Feb 11, 2019

@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

This comment has been minimized.

Copy link
Member

Mariatta commented Feb 11, 2019

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

@ned-deily

This comment has been minimized.

Copy link
Member

ned-deily commented Feb 11, 2019

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

This comment has been minimized.

Copy link
Member

Mariatta commented Feb 11, 2019

🤔 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

This comment has been minimized.

Copy link
Member

ned-deily commented Feb 11, 2019

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.

@paulmon paulmon force-pushed the paulmon:openssl-111a branch from 6211939 to ac987a1 Feb 28, 2019

@tiran

This comment has been minimized.

Copy link
Member

tiran commented Feb 28, 2019

Please update to OpenSSL 1.1.1b.

@tiran

This comment has been minimized.

Copy link
Member

tiran commented Feb 28, 2019

Also see PR #12094

Since we have to update 3.7 to 1.1.1 in about half a year, I no longer mind to backport your improvements after 3.7.4 is out. It's @ned-deily's call.

@paulmon

This comment has been minimized.

Copy link
Contributor Author

paulmon commented Mar 1, 2019

@zooba can you update the OpenSSL sources and binaries to OpenSSL 1.1.1b for Windows?

@paulmon

This comment has been minimized.

Copy link
Contributor Author

paulmon commented Mar 2, 2019

Is there a right way to test openssl?
I have been using this command line, but I don't remember where I found it.
.\python.bat -Werror -bb -m test -u urlfetch -u network -v test_ssl

@paulmon

This comment has been minimized.

Copy link
Contributor Author

paulmon commented Mar 2, 2019

The last commit won't pass CI until cpython-bin-deps is updated to openssl 1.1.1b

@zooba

This comment has been minimized.

Copy link
Member

zooba commented Mar 7, 2019

@paulmon I just pushed openssl-bin-1.1.1b branch (and sources) to the repo, so you should be unblocked here now.

@bedevere-bot

This comment has been minimized.

Copy link

bedevere-bot commented Mar 10, 2019

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@paulmon paulmon force-pushed the paulmon:openssl-111a branch from 835cf26 to bade87e Mar 11, 2019

@paulmon paulmon changed the title bpo-35926: Add support for OpenSSL 1.1.1a on Windows bpo-35926: Add support for OpenSSL 1.1.1b on Windows Mar 11, 2019

@paulmon

This comment has been minimized.

Copy link
Contributor Author

paulmon commented Mar 11, 2019

The code has been updated to OpenSSL 1.1.1b and all of the tests pass on my machine with the changes here to catch new exceptions. What are the next steps for getting this merged?

@zooba

This comment has been minimized.

Copy link
Member

zooba commented Mar 11, 2019

I think we need @tiran to take another look and sign off.

@paulmon

This comment has been minimized.

Copy link
Contributor Author

paulmon commented Mar 18, 2019

@ned-deily the stale .rst file has been removed as requested.
@tiran can you sign off? Or is more work needed?
Thanks, Paul

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.