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
requests.packages monkey patching of unbundled urllib3 does not work #3287
Comments
Here is a set of tests which demonstrate some of the problem: https://travis-ci.org/jayvdb/requests/builds/135716555 My apologies that it doesnt show a problem in Instead, see the errors in
and yet they have warnings indicating that those requests are not being processed securely. |
@jayvdb thanks for testing but there is something I don't get. Tests for |
Hi @eriol, the As a consequence of building the additional testing on Travis, I feel more confident that any distro version shipping Python 2.7.9+ is fine with the current So the only 'problem' may be that the requests 2.10 package in stretch / sid states it Arch Linux also uses the unbundling, but it appears to be only providing Python 3.5. |
@eriol, I've been able to provide a better test case for this, and it shows that the problem exists even in 2.7.11 and 3.3. https://travis-ci.org/jayvdb/requests/builds/135867398 is just #3289 with a https://travis-ci.org/jayvdb/requests/builds/135870520 is a very simple change to the Finally, https://travis-ci.org/jayvdb/requests/builds/135873388 is #3286 , which fixes the problem. My next step is going to be to check what happens with symlinks like what Fedora is doing, to see if that is a way to beat the import machinery. (I'd be surprised if it didnt work, but this problem is full of surprises). |
symlink results are in, with failures up until 2.7.8 only, and the failures are the opposite -- there are too many warnings, rather than too few. |
I've put together a little playground to show what is happening under the covers, as I found it difficult to do this under pytest, in order to better see the scope of the problem. Worth noting that 2.7.9+ seems to mostly do the right thing, but I havent yet added any demos using site using/abusing Subject Alternative Name. |
@warsaw , as I saw you're working on a similar package and de-vendoring ( pypa/pip#3802 ), you might like to be aware of, or assist with, this. |
@jayvdb Thanks for the heads-up. That was a quick fix which seemed to work for Debian. I'll take a look at this issue in more detail when I get a chance. |
Hi, i did non forgot this. Only busy during this week. @warsaw my was plan is to use same approach of pip here. What do you think about? |
Once #3289 is merged, we'll have a unit test to check the de-vendored solution. |
We've gone through many iterations of the debundling with pip. I think we're pretty happy with the current incantation which has the following properties:
The downsides that we've discovered so far:
|
@dstuff, I very quickly tested pip, and it inherits this problem from requests. pip de-vendoring may well be working wonderfully, but the de-vendored requests isnt initalising urlllib3's pyopenssl properly. See https://travis-ci.org/jayvdb/requests_issue_3287/builds/138597348 , which shows only additional warnings being emitted for pip's requests, but please see the rest of this issue to see why those warnings are occurring and the impact of that. Due to pip mostly being used to talk to one set of servers, the fallout is smaller. My guess is that this isnt a security vulnerability , as it will cause connections to fail when they should succeed, rather than connect when it shouldnt. But I have very little experience in this area, and not much time to understand the corner cases in these packages, so I am sure others here will know the real potential for this to be abused or not. The pip travis testing framework for de-vendored packages is nice, but note that https://github.com/pypa/pip/blob/master/pip/_vendor/vendor.txt isnt including |
@jayvdb fast report, working on unbundling stuff right now. Testing with your https://github.com/jayvdb/requests_issue_3287. |
@dstufft many thanks for the detailed explanation! I have just uploaded @jayvdb can you test again when One more thing, I can bump the Python dependency to ensure a cPython2 with SSL feature from Python3, but only after we fix this. I was not aware of |
I have limited capacity to test atm, in a bit of a rush to airport, but should be good in ~6 hrs. Regarding issue title, this problem also occurs only py3. |
Also there are other packages that requests vendors the same way, so obscure problems can probably be found there also. It is the monkey patching that is wrong, not an ssl issue. It is just the ssl side effect is the most concerning of course. |
I've done some basic testing. The warnings are fixed. It appears that Debian's 2.10.0-2 However You can see Debian's requests 2.10.0-2 has different state to urllib3 at https://travis-ci.org/jayvdb/requests_issue_3287/builds/138669983#L289 , but that is because requests on Debian now copies urllib3 and doesnt enable pyopenssl in the original top level urllib3 package which is left in its default state as my test script doesnt attempt to enable pyopenssl in urllib3. |
Hi, distro user here, and today I've met exactly the same issue. Can someone explain to me why requests bundles so many packages? For easy-of-use? Users need only one big package in this way. But pip has done a good job on installing dependencies for these users. So it is for pinning known-compatible versions? Or something else I've totally missed? |
@lilydjwg pip does a good job on installing dependencies if there are no conflicts. Consider the following case: There's a version of Requests, let's call it
Then they will have a broken Requests installation. Vendoring the dependencies makes it easy to know that Requests will always work when installed from PyPI (we cannot ever make the same guarantee for a distro user, that guarantee is up to the packagers to make). Both Requests and urllib3 are widely used and popular packages. Requests will sometimes want to stick to an older version or use a newer version and this allows us complete control over that. Keep in mind, however, that pip has GSOC student who is working on resolving the above problem. That will be one less issue for Requests to worry about then once that lands. Unfortunately, people don't often upgrade pip. Namely, the people who rarely update pip are users such as yourself who rely on their distro to provide them python packages. In your case, you will likely run into this issue unless you're using a alpha/beta version of your distro or something like Arch Linux which does it's best to stay current. |
@sigmavirus24 thanks for the explanation! I'm on Arch Linux. However companies tend to use stable distros with terribly-old packages, so I install pip and upgrade it to latest every time :-) |
Closing since Requests unvendored everything recently. |
data['created_by'] = request.headers.get(header.GRASS_HEADER_USER_ID, '') |
@sakshi094 that's not what this issue is about. I suggest you ask your question on StackOverflow. |
In requests 2.5.2
requests.packages.__init__
added code to allowrequests.packages.urllib3
to not exist, and it would fall back to usingurllib3
.No doubt this works in some cases, as the people on #2375 were happy with it, but it doesnt work with pyopenssl, resulting in no SNI/etc.
requests.packages.__init__
only injectsurllib3
asrequests.packages.urllib3
.However all of the other modules within urlllib3 will have different copies in the namespace
requests.packages.urllib3
.requests.packages.urllib3.contrib.pyopenssl
then writes to a separate copy ofrequests.packages.urllib3.util
andrequests.packages.urllib3.connection
, and these changes are never seen byurllib3
.I encountered this on debian stretch & sid's
python-requests
2.10.0-1 withpython-urllib3
1.15.1 when running on Python 2.7.6 (Ubuntu Trusty), and haven't retested yet on a later Python 2.7. @eriol#3286 fixes the problem for me.
I note that Fedora's latest
python-requests
no longer unbundles urllib3.The text was updated successfully, but these errors were encountered: