-
Notifications
You must be signed in to change notification settings - Fork 239
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
Conversation
dd678ee
to
d63ebdd
Compare
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 Apart from that, if it solves the problem, this seems like a rather isolated fix, so that's always a plus. |
@YannickJadoul
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:
Here's a build showing tests without patch: https://travis-ci.org/mayeut/cibuildwheel/builds/375108210
Another job skipping macOS 3.4 to check 3.5 behavior: https://travis-ci.org/mayeut/cibuildwheel/builds/375114371
Well, it's isolated in terms of code added to |
It would be nice if the patch was at least part of this repo. |
@mayeut Thanks a lot for that explanation; that does clear up things!
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?
I agree it would be nice, but given that the files are quite large, it seems not optimal to pack them alongside A few, more questions, to get a better idea of what to do:
|
Well the test try to create an SSL context forcing TLS 1.2
Yes, isolated repo seems good to me as well. I had in mind that if there was a
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.
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 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
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 |
Oh, right, hadn't looked that far, but that's amazing!
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.) |
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 @mayeut, if you're happy to keep the So, I'm in favour of merging this, unless anyone has any more comments? |
@joerick, I'm OK with that. |
Sorry it's taken so long for me to get to this, but thanks again @mayeut, cutting a release with this now :) |
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:pip
is updated bycibuildwheel
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.2There 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:
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.