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

macOS: patch _ssl module on Python 3.4 & 3.5 #71

Merged
merged 2 commits into from
Jun 18, 2018
Merged

Conversation

mayeut
Copy link
Member

@mayeut mayeut commented May 6, 2018

This is work in progress to address #70 (and #63 though the issue in this one can probably be worked around)

As of now, cibuildwheel uses:

  • python 2.7.15 official installer, ships with OpenSSL 1.0.2o
  • python 3.4.4 official installer, uses system OpenSSL 0.9.8zh
  • python 3.5.4 official installer, uses system OpenSSL 0.9.8zh
  • python 3.6.5 official installer, ships with OpenSSL 1.0.2n

pip is updated by cibuildwheel to the latest version and this latest now has a SecureTransport fallback implementation, thus, it works properly even when OpenSSL does not support TLS 1.2

There are now issues when building or testing wheels on macOS python 3.4 & 3.5 when build and/or test uses python ssl module (either directly or indirectly) as can be seen in #70 or #63

The easy way would be to ask python to do the same for 3.4 & 3.5 but official installer are no built anymore for those "security fix" only versions (c.f. https://www.python.org/downloads/release/python-348/ & https://www.python.org/downloads/release/python-355/)

In order to get OpenSSL to a decent version for python 3.4 & 3.5, I can see multiple options:

  1. Use a custom built python (that still provides intel universal support...)
  2. Patch an existing install

Well, I tried to back port the installer creation from 3.6.5 to 3.5.5 but I couldn't figure out how to make this work on my setup. The python documentation for building a pkg installer is pretty much non-existent or I couldn't find it (build-installer.py will only build a dmg image rather than a pkg installer).

Thus, I went for option number 2. This PR uses patches from https://github.com/mayeut/patch-macos-python-openssl

I also added a test for SSL (this required to build on top of #66 c.f. https://travis-ci.org/mayeut/cibuildwheel/jobs/375098431, because of sudo, cibuildwheel runs on an old python2 provided by the system rather than up-to-date python2 provided by travis)

As said in the title it's work in progress.
I won't work on that anymore unless @YannickJadoul, @joerick have some insight on what shall be done next.

@YannickJadoul
Copy link
Member

Hmmm, for some reason, I am finding it hard to wrap my head around this problem. But I am quite impressed with your solution and PR. Thank you very much for looking into this!

I find it hard to review the changes by myself, but I'm assuming these tests would be failing with a version of cibuildwheel without this PR?

Apart from that, if it solves the problem, this seems like a rather isolated fix, so that's always a plus.

@mayeut
Copy link
Member Author

mayeut commented May 6, 2018

@YannickJadoul
I will try to answer your 3 remarks here

Hmmm, for some reason, I am finding it hard to wrap my head around this problem. But I am quite impressed with your solution and PR. Thank you very much for looking into this!

If the explanations are not enough in the PR description, I'd be glad to give more but as of now, I don't know what more to say except the following:
Many secure servers are now restricting connections to TLS v1.1 and/or TLS v1.2 in order to enforce security (you can see the evolution in time there https://www.ssllabs.com/ssl-pulse/). PyPI now enforce TLS v1.2 for example
Apple historically provides OpenSSL 0.9.8 which has no support for these protocols and that's what official Python 3.4 & 3.5 macOS installers are using (2.7.14 and less also, 2.7.15 changed that c.f. #68).
When PyPI started to move to TLS v1.2, they had so many issues with mac users that they implemented a workaround in pip to bypass the python ssl module and use SecureTransport instead. The fix of cibuildwheel to get a working pip version has been merged in #57.
Now, issues still arise when "official" Python 3.4 or 3.5 ssl module is used to connect to a server enforcing TLS v1.1 or v1.2
This PR solves those issues by applying a patch to those versions using a rebuilt _ssl (the private native module ssl module relies on) that links against OpenSSL 1.0.2o (provided as a shared library in the patch)

I find it hard to review the changes by myself, but I'm assuming these tests would be failing with a version of cibuildwheel without this PR?

Here's a build showing tests without patch: https://travis-ci.org/mayeut/cibuildwheel/builds/375108210
The interesting part is:

Requirement already satisfied: wheel in /Library/Frameworks/Python.framework/Versions/3.4/lib/python3.4/site-packages (from delocate) (0.31.0)
+ pip3 wheel /Users/travis/build/mayeut/cibuildwheel/test/07_ssl -w /tmp/built_wheel --no-deps
Processing ./test/07_ssl
    Complete output from command python setup.py egg_info:
    Traceback (most recent call last):
      File "<string>", line 1, in <module>
      File "/private/var/folders/bb/n7t3rs157850byt_jfdcq9k80000gn/T/pip-req-build-4c7kreki/setup.py", line 13, in <module>
        context = ssl.SSLContext(ssl.PROTOCOL_TLSv1_2)
    AttributeError: 'module' object has no attribute 'PROTOCOL_TLSv1_2'
    
    ----------------------------------------
Command "python setup.py egg_info" failed with error code 1 in /private/var/folders/bb/n7t3rs157850byt_jfdcq9k80000gn/T/pip-req-build-4c7kreki/

Another job skipping macOS 3.4 to check 3.5 behavior: https://travis-ci.org/mayeut/cibuildwheel/builds/375114371
With the interesting part:

Requirement already satisfied: wheel in /Library/Frameworks/Python.framework/Versions/3.5/lib/python3.5/site-packages (from delocate) (0.31.0)
+ pip3 wheel /Users/travis/build/mayeut/cibuildwheel/test/07_ssl -w /tmp/built_wheel --no-deps
Processing ./test/07_ssl
    Complete output from command python setup.py egg_info:
    Traceback (most recent call last):
      File "<string>", line 1, in <module>
      File "/private/var/folders/bb/n7t3rs157850byt_jfdcq9k80000gn/T/pip-req-build-gll7i67_/setup.py", line 13, in <module>
        context = ssl.SSLContext(ssl.PROTOCOL_TLSv1_2)
    AttributeError: module 'ssl' has no attribute 'PROTOCOL_TLSv1_2'
    
    ----------------------------------------
Command "python setup.py egg_info" failed with error code 1 in /private/var/folders/bb/n7t3rs157850byt_jfdcq9k80000gn/T/pip-req-build-gll7i67_/

Apart from that, if it solves the problem, this seems like a rather isolated fix, so that's always a plus.

Well, it's isolated in terms of code added to cibuildwheel however there's an additional external dependency: the patch
If you and @joerick are interested in getting this PR merged in, I think the way the patch is built and deployed shall be a part of cibuildwheel somehow. Let's discuss this some more only if you're interested.

@hroncok
Copy link
Contributor

hroncok commented May 6, 2018

It would be nice if the patch was at least part of this repo.

@YannickJadoul
Copy link
Member

@mayeut Thanks a lot for that explanation; that does clear up things!

Here's a build showing tests without patch: https://travis-ci.org/mayeut/cibuildwheel/builds/375108210

So if I understand correctly now, that is because TLS v1.2 is not implemented there (i.e. a Python error in the library), while for #70, it is because the server is rejecting a version that is too old?

If you and @joerick are interested in getting this PR merged in, I think the way the patch is built and deployed shall be a part of cibuildwheel somehow. Let's discuss this some more only if you're interested.

I agree it would be nice, but given that the files are quite large, it seems not optimal to pack them alongside cibuildwheel. And to use the GitHub releases of this same repository seems quite confusing/hard. So a separate, isolated repository as you have might actually be quite good? Or how what did you have in mind?

A few, more questions, to get a better idea of what to do:

  • How often do you think this build script for the patch would need to be re-run? I would assume that a new patch is rarely necessary, and that a manual run would be OK?
  • Python has no immediate plans to fix this? And it is becoming a well-known problem for older distributions? Is there a way we could pack your patch and publish it through pip (I'd expect not, since you're overwriting the built-in libraries, right?) ? Or, if this is a maintenance-low project anyway, just keeping it as a separate project might still be attractive? (Surely it's not perfect, but for Windows, we're also relying on this external project/file.)

@mayeut
Copy link
Member Author

mayeut commented May 7, 2018

So if I understand correctly now, that is because TLS v1.2 is not implemented there (i.e. a Python error in the library), while for #70, it is because the server is rejecting a version that is too old?

Well the test try to create an SSL context forcing TLS 1.2 context = ssl.SSLContext(ssl.PROTOCOL_TLSv1_2). In #70, the context is not forced, it's a negotiation failure because TLS 1.2 is not supported. The root cause is the same (no support of TLS 1.2) but it shows in different ways.

So a separate, isolated repository as you have might actually be quite good? Or how what did you have in mind?

Yes, isolated repo seems good to me as well. I had in mind that if there was a cibuildwheel organization, I could transfer ownership of my patch repo to this organization making it clear where it's being used.

How often do you think this build script for the patch would need to be re-run? I would assume that a new patch is rarely necessary, and that a manual run would be OK?

I think it should be re-run when a new OpenSSL patch gets out. In the past, it's been between 4 & 6 times a year. Everything's already fully automated in the patch repo. It's a matter of pushing the OpenSSL version / SHA256 and create a tag.

Python has no immediate plans to fix this? And it is becoming a well-known problem for older distributions?

It's now fixed in 2.7 and 3.6 official installers which are still actively maintained. The issue in Python bug tracker has been closed. 3.4 & 3.5 are source only security bug-fix now.
I think, but I may be mistaken, that all that matters to users of those official installers is to have a 2.x or 3.x python install that works so it seems reasonable not to update 3.4 & 3.5 installers. Other users will probably be using homebrew, conda, pyenv, ... installs which are not affected.
cibuildwheel on the other hand does require the official 3.4 and 3.5 installers in order to build intel universal wheels. I honestly think this is the only use case that still requires those installers.

Is there a way we could pack your patch and publish it through pip (I'd expect not, since you're overwriting the built-in libraries, right?) ?

I thought of that for a second but I'm not sure it's something that can be done. Let's keep this for later if needed but I don't expect the patch to get much traction except from people building wheels and requiring TLS 1.2 support for something other than pip.

Or, if this is a maintenance-low project anyway, just keeping it as a separate project might still be attractive? (Surely it's not perfect, but for Windows, we're also relying on this external project/file.)

I don't mind keeping my repo around. In fact, as said earlier and as you said yourself, a separate repo is probably the way to go. As I said, a cibuildwheel organization might help to get a consistent overview of the main project dependencies. If that's too much of a burden for only one dependency, never mind, my repo won't be deleted (and I can give cibuildwheel maintainers access to it if needed).
BTW, it troubled me when I first saw that windows dependency in the code base.

@YannickJadoul
Copy link
Member

Everything's already fully automated in the patch repo. It's a matter of pushing the OpenSSL version / SHA256 and create a tag.

Oh, right, hadn't looked that far, but that's amazing!

As I said, a cibuildwheel organization might help to get a consistent overview of the main project dependencies.

That might indeed be the cleanest solution, but let's wait for what @joerick has to say, once he's got more time. And let's also pull @tgarc into the conversation, then.

Until then, I propose to keep things as they currently are? If you don't mind holding on to the repository for the next few weeks, that's probably the easiest. The one thing we should decide on is whether this PR would to be merged and released? Because if this is really only for projects with tests that need TSL, we could also refer them to using this PR's branch until a more final solution is found and merged? (But I'm happy to merge this and see if we can get a temporary release going, as well.)

@joerick
Copy link
Contributor

joerick commented May 8, 2018

Hi all, I'm just catching up on this. First of all, phenomenal work so far @mayeut! Really happy to see such a high-quality contribution :) And thank you @YannickJadoul for the thorough review.

This is a tricky problem, I know. I do agree with the patching approach you've provided for solving the problem.

My first reaction was wether this could be pip-installed too, and perhaps be more generally useful. I did a little work on this but my main problem is that lib-dynload is higher up on sys.path than site-packages so my _ssl.so isn't getting imported. Maybe can work around with the wheel data folder, but then would have to figure out the install_name_tool incantation to do relative linker paths. So I'm happy to shelve this for now.

@mayeut, if you're happy to keep the patch-macos-python-openssl repo around, I'm cool with keeping things as they are now. That dependency shouldn't require many updates, the build script is pretty small, and we're able to switch the URL to a different repo pretty easily. I've kept a fork incase you delete all your repos in a freak accident ;)

So, I'm in favour of merging this, unless anyone has any more comments?

@mayeut mayeut changed the title WIP macOS: patch _ssl module on Python 3.4 & 3.5 macOS: patch _ssl module on Python 3.4 & 3.5 May 8, 2018
@mayeut
Copy link
Member Author

mayeut commented May 8, 2018

@joerick, I'm OK with that.

@joerick
Copy link
Contributor

joerick commented Jun 18, 2018

Sorry it's taken so long for me to get to this, but thanks again @mayeut, cutting a release with this now :)

@joerick joerick merged commit 189dc13 into pypa:master Jun 18, 2018
@mayeut mayeut deleted the macos-ssl branch June 19, 2018 16:11
@mayeut mayeut mentioned this pull request Oct 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants