Enable vendor C libraries on non-Windows platforms #4835

pradyunsg · 2017-11-04T07:35:41Z

Closes #4612 (this is that PR updated)
Closes #4098 (original issue)
Closes #4142 (competing PR)

This essentially allows users with Py <2.7.9 to fix security warnings by upgrading the relevant packages, using pip on non-Windows platforms.

pradyunsg · 2017-11-04T07:36:08Z

Some history:

@dstufft in #4098 (comment):

So this is a bit of a sticky issue. We've modified our bundled copies of the libraries so that they will not load any of the C libraries because on some OSs (particularly Windows) if pip imports the C library then it becomes impossible for pip to actually upgrade or uninstall that library (because importing locks the .dll from deletion). The downside of this is that it means you're stuck with what your Python is able to provide.

I see a few ways around this:

Do nothing, let the warning's stand to try and push people to upgrade their Python to one that has a better SSL module.

Disable the warnings completely, the warnings don't matter much for PyPI's own usage (although they could for non PyPI repositories) and just live with it.

Adjust our disable of C libraries to only disable them on platforms where they cause problems (e.g. Windows).

@dstufft in #4098 (comment):

If someone makes a PR for (1) and (3) I would be happy to accept it

@alex in #4142 (comment):

Thing that just occurred to me: If pip starts supporting loading C-extensions again, we probably ought to make the openssl_version in the User-Agent reflect the version loaded by cryptography, not the one loaded by Python's ssl module.

pradyunsg · 2017-11-04T07:37:45Z

I'm not sure what has to be done for the point that @alex raised.

My intuition is that all of pip's traffic goes through requests which would somehow use the correct user-agent. I've not verified this though.

xavfernandez

Regarding @alex 's ssl-point this would happen here:

pip/src/pip/_internal/download.py

Lines 121 to 122 in fb56eb9

    
           if HAS_TLS: 
        
               data["openssl_version"] = ssl.OPENSSL_VERSION

But I'm not sure we want to change that, the openssl version of the ssl module gives an interesting statistic and the openssl version used to actually download packages would be an other one.

dstufft · 2017-11-04T23:32:10Z

I think we'd want to expose the version of OpenSSL we're actually using to talk to the internet with, although I'm not sure offhand how to do that with cryptography or how to know if requests is going to use ssl or pyopenssl.

pradyunsg · 2017-11-05T10:54:00Z

I came up with something. Relavent lines that make me think this is what we want to do:

urllib3/util/ssl_.py:14
urllib3/contrib/pyopenssl.py:117
requests/help.py:L76

There have since been changes made.

pradyunsg · 2017-11-07T10:13:09Z

https://github.com/pradyunsg/pip/blob/945196e5f0dcf47279d328a6bbdaef11324c2e4d/src/pip/_internal/__init__.py#L23

What do we want to do about these lines?

PS: On MacOS with OpenSSL < 1.0.1, were we posting the wrong version of openssl due to the fact that urllib3 was injected but not actually used?

(edit: rephrased as a question)

pradyunsg · 2017-11-08T15:08:10Z

What do we want to do about these lines?

🔥 them.

(edit: no dum dum me, they stay)

dstufft · 2017-11-08T15:15:48Z

src/pip/_internal/__init__.py

-    if (sys.platform == "darwin" and
-            ssl.OPENSSL_VERSION_NUMBER < 0x1000100f):  # OpenSSL 1.0.1
-        try:
-            from pip._vendor.urllib3.contrib import securetransport


Why are we removing this? This is needed to support macOS < High Sierra once PyPI is TLSv1.2+.

alex · 2017-11-08T15:16:29Z

Your most recent commit appears to totally disable SecureTransport -- I don't see any other code which enables it.

Am I misreading?

pradyunsg · 2017-11-08T15:17:58Z

Whoops. I misread the import. :/

pradyunsg · 2017-11-08T15:18:30Z

Reverted.

pradyunsg · 2017-11-12T14:30:01Z

Is there anything outstanding on this?

dstufft · 2018-03-16T04:10:09Z

src/pip/_internal/download.py

@@ -46,11 +48,22 @@
 from pip._internal.utils.ui import DownloadProgressProvider
 from pip._internal.vcs import vcs

+# We don't try to import OpenSSL on Windows since that might result in it
+# loading C libraries, which can then not be uninstalled.
+if not WINDOWS:


It might be a good idea to not duplicate this logic, and instead use the variables that urllib3 sets, see:

https://github.com/shazow/urllib3/blob/master/urllib3/contrib/pyopenssl.py#L109-L118

orig_util_SSLContext is never None -- https://github.com/shazow/urllib3/blob/master/urllib3/util/ssl_.py#L90

I do think calling _validate_dependencies_met is a good idea.

dstufft · 2018-03-16T04:10:31Z

Looks OK to me, one comment about duplicating some logic.

pfmoore · 2018-03-26T13:59:06Z

@pradyunsg Are you planning on merging this? There's a minor review commend from @dstufft to consider.

pradyunsg · 2018-03-30T07:57:23Z

I am. One minute.

alex · 2018-03-30T12:49:20Z

src/pip/_internal/download.py

@@ -119,7 +137,10 @@ def user_agent():
        data["cpu"] = platform.machine()

    if HAS_TLS:
-        data["openssl_version"] = ssl.OPENSSL_VERSION
+        if IS_PYOPENSSL:
+            data["openssl_version"] = OpenSSL.SSL.OPENSSL_VERSION_NUMBER


This is wrong:

>>> import OpenSSL >>> OpenSSL.SSL.OPENSSL_VERSION_NUMBER 269484175 >>> import ssl >>> ssl.OPENSSL_VERSION 'OpenSSL 1.0.2o 27 Mar 2018'

I actually believe we should roll this back entirely and always send ssl.OPENSSL_VERSION, so the data is consistent. Sending different data in ways that can't be detected in teh database is a bad idea.

Yea, after talking to Alex I think I agree.

If there's some way to make a string representation of the version number, would that be fine?

alex · 2018-03-30T13:23:34Z

No, I think we should always do the ssl module version

…

On Fri, Mar 30, 2018, 9:18 AM Pradyun Gedam ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/pip/_internal/download.py <#4835 (comment)>: > @@ -119,7 +137,10 @@ def user_agent(): data["cpu"] = platform.machine() if HAS_TLS: - data["openssl_version"] = ssl.OPENSSL_VERSION + if IS_PYOPENSSL: + data["openssl_version"] = OpenSSL.SSL.OPENSSL_VERSION_NUMBER If there's some way to make a string representation of the version number, would that be fine? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4835 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAADBBVwUZ_EZoTa-xR2M_s1RLP3O252ks5tjjC4gaJpZM4QR7GI> .

pradyunsg · 2018-03-30T15:23:51Z

Awesome. That'll make this PR much lighter.

Made a change.

pfmoore · 2018-03-30T21:23:19Z

@pradyunsg I assume you'll be merging this soon? I'll be cutting the 10.0 beta in about 12 hours (Saturday morning UK time).

This and the author cleanup (#5118) are basically the last remaining PRs that I'm expecting to go in to 10.0.

pradyunsg · 2018-03-30T21:29:56Z

And, yeah, I expect this to be the last thing in pip 10.0 beta (and 10.0 if there's no issues reported). I guess I'll have 2 weeks for triaging all the issues once we hit the freeze. :) I'm cool if someone else clicks merge on this.

…

On Sat, 31 Mar 2018, 02:55 Pradyun Gedam, ***@***.***> wrote: Just merged the author cleanup after verifying locally. I'll wait on Alex and Donald for this. I do think this is good to go. On Sat, 31 Mar 2018, 02:53 Paul Moore, ***@***.***> wrote: > @pradyunsg <https://github.com/pradyunsg> I assume you'll be merging > this soon? I'll be cutting the 10.0 beta in about 12 hours (Saturday > morning UK time). > > This and the author cleanup (#5118 > <#5118>) are basically the last > remaining PRs that I'm expecting to go in to 10.0. > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#4835 (comment)>, or mute > the thread > <https://github.com/notifications/unsubscribe-auth/ADH7SRQm_14uXP7HVdunOHVIuYjKNmb2ks5tjqJLgaJpZM4QR7GI> > . >

pradyunsg · 2018-03-30T21:29:56Z

Just merged the author cleanup after verifying locally. I'll wait on Alex and Donald for this. I do think this is good to go.

…

On Sat, 31 Mar 2018, 02:53 Paul Moore, ***@***.***> wrote: @pradyunsg <https://github.com/pradyunsg> I assume you'll be merging this soon? I'll be cutting the 10.0 beta in about 12 hours (Saturday morning UK time). This and the author cleanup (#5118 <#5118>) are basically the last remaining PRs that I'm expecting to go in to 10.0. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4835 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADH7SRQm_14uXP7HVdunOHVIuYjKNmb2ks5tjqJLgaJpZM4QR7GI> .

pfmoore · 2018-03-30T21:33:16Z

OK. I'll be offline now until tomorrow morning, so I'll check back on this PR then. We've had an approval; from @alex so once @dstufft approves it, it can go. I can do the merge if you're not around.

pradyunsg · 2018-03-30T21:34:11Z

I'm gonna go offline too. It's 3am. :P

…

On Sat, 31 Mar 2018, 03:03 Paul Moore, ***@***.***> wrote: OK. I'll be offline now until tomorrow morning, so I'll check back on this PR then. We've had an approval; from @alex <https://github.com/alex> so once @dstufft <https://github.com/dstufft> approves it, it can go. I can do the merge if you're not around. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4835 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADH7SbLL3F4ElXGXPQX8RAoL3PCyDbZNks5tjqSfgaJpZM4QR7GI> .

amp343 · 2018-03-31T00:15:00Z

👍 @pradyunsg thanks for working this through

lock · 2019-06-02T12:05:35Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

pip and others added 2 commits July 19, 2017 14:26

Limit the disabling of requests' pyopenssl import to Windows

2f76fbe

Merge branch 'master' into pr/4612

0320093

pradyunsg added type: enhancement Improvements to functionality type: security Has potential security implications project: vendored dependency Related to a vendored dependency labels Nov 4, 2017

pradyunsg added this to the 10.0 milestone Nov 4, 2017

pradyunsg mentioned this pull request Nov 4, 2017

SNIMissingWarning / InsecurePlatformWarning not fixable with pip 9.0 / 9.0.1 #4098

Closed

pradyunsg changed the title ~~Vendoring/enable c libs~~ Enable vendor C libraries on non-Windows platforms Nov 4, 2017

pradyunsg force-pushed the vendoring/enable-c-libs branch from c187f2f to 0a2c524 Compare November 4, 2017 07:43

Fix a mistake in merge

88924a4

pradyunsg force-pushed the vendoring/enable-c-libs branch from 0a2c524 to 88924a4 Compare November 4, 2017 07:45

xavfernandez previously approved these changes Nov 4, 2017

View reviewed changes

User-Agent reflects the version of openssl used to connect

0c684a3

pradyunsg force-pushed the vendoring/enable-c-libs branch from f4f6fe7 to 0c684a3 Compare November 5, 2017 10:55

Merge branch 'master' into vendoring/enable-c-libs

945196e

pradyunsg requested a review from a team November 8, 2017 15:13

dstufft reviewed Nov 8, 2017

View reviewed changes

pradyunsg force-pushed the vendoring/enable-c-libs branch from c61e1b6 to 945196e Compare November 8, 2017 15:18

pradyunsg requested a review from a team November 8, 2017 15:19

dstufft reviewed Mar 16, 2018

View reviewed changes

Merge branch 'master' into vendoring/enable-c-libs

6ec5bc5

Reuse logic from urllib3 for use of OpenSSL

aaa9310

pradyunsg requested a review from a team March 30, 2018 08:04

pradyunsg self-assigned this Mar 30, 2018

dstufft previously approved these changes Mar 30, 2018

View reviewed changes

alex reviewed Mar 30, 2018

View reviewed changes

pradyunsg closed this Mar 30, 2018

pradyunsg reopened this Mar 30, 2018

Stop bothering about the PyOpenSSL version

bd31672

alex approved these changes Mar 30, 2018

View reviewed changes

dstufft merged commit 0007825 into pypa:master Mar 30, 2018

pradyunsg deleted the vendoring/enable-c-libs branch April 1, 2018 07:18

pradyunsg restored the vendoring/enable-c-libs branch May 27, 2018 09:38

pradyunsg deleted the vendoring/enable-c-libs branch May 27, 2018 10:08

lock bot added the auto-locked Outdated issues that have been locked by automation label Jun 2, 2019

lock bot locked as resolved and limited conversation to collaborators Jun 2, 2019

Enable vendor C libraries on non-Windows platforms #4835

Enable vendor C libraries on non-Windows platforms #4835

Uh oh!

Conversation

pradyunsg commented Nov 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pradyunsg commented Nov 4, 2017

Uh oh!

pradyunsg commented Nov 4, 2017

Uh oh!

xavfernandez left a comment

Choose a reason for hiding this comment

Uh oh!

dstufft commented Nov 4, 2017

Uh oh!

pradyunsg commented Nov 5, 2017

Uh oh!

pradyunsg commented Nov 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pradyunsg commented Nov 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alex commented Nov 8, 2017

Uh oh!

pradyunsg commented Nov 8, 2017

Uh oh!

pradyunsg commented Nov 8, 2017

Uh oh!

pradyunsg commented Nov 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dstufft commented Mar 16, 2018

Uh oh!

pfmoore commented Mar 26, 2018

Uh oh!

pradyunsg commented Mar 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alex commented Mar 30, 2018 via email

Uh oh!

pradyunsg commented Mar 30, 2018

Uh oh!

pfmoore commented Mar 30, 2018

Uh oh!

pradyunsg commented Mar 30, 2018 via email

Uh oh!

pradyunsg commented Mar 30, 2018 via email

Uh oh!

pfmoore commented Mar 30, 2018

Uh oh!

pradyunsg commented Mar 30, 2018 via email

Uh oh!

amp343 commented Mar 31, 2018

Uh oh!

lock bot commented Jun 2, 2019

Uh oh!

Uh oh!

pradyunsg commented Nov 4, 2017 •

edited

Loading

pradyunsg commented Nov 7, 2017 •

edited

Loading

pradyunsg commented Nov 8, 2017 •

edited

Loading