Clean rebased version of https-proxy #170

schlamar · 2013-04-05T09:46:51Z

This is a cleaned up version of #139 rebased against current master.

Current todo:

All discussions in the diff

proxy* private: Clean rebased version of https-proxy #170 (diff)
ProxyManager API Clean rebased version of https-proxy #170 (diff)
PoolManager attributes: Clean rebased version of https-proxy #170 (diff)
extract connection related stuff: Clean rebased version of https-proxy #170 (diff)
PEP8: Clean rebased version of https-proxy #170 (diff)
comment: Clean rebased version of https-proxy #170 (review)
style: Clean rebased version of https-proxy #170 (review)

Other

HTTPS proxy support (issue #50) #139 Bug if SSL is not available.
Clean rebased version of https-proxy #170 TODO comments. I think only cleaning them up is relevant here, proxy auth can be addressed separately. The best would be to create an issue for it so it won't be forgotten :)
Clean rebased version of https-proxy #170 Failing tests on Windows.
Clean rebased version of https-proxy #170 (comment) Address in use on Travis

Lukasa · 2013-04-23T07:23:07Z

Is there anything I can do to help keep this moving along, @shazow? I appreciate you have lots of pressure on your time, so if there's anything you'd like me to do please say. =)

shazow · 2013-04-23T17:35:02Z

Hm so what's the status of this? Is it ready to be considered for merging again?

I'm still seeing a bunch of # TODO's, is that intended?

Anyone familiar with software copyright/licenses have insight on whether it's a problem to introduce separately licensed submodules into the main codebase? (dummyserver/proxy.py) Should we try to consolidate the licenses?

Anorov · 2013-04-23T22:52:00Z

The only TODO of significance left is the "proxy authentication" one, I think.

Personally I think all proxy auth should be handled in the URL, with maybe an alternate option of passing it into the proxy_headers constructor parameter. That is, https://username:password@proxy.com. The username and password can then be accessed from self.proxy.auth, since it is urlparsed.

How is basic auth handled normally? I see a make_headers function in util.py but it doesn't seem to be used within the library itself.

shazow · 2013-04-24T05:49:49Z

urllib3's level of abstraction does not hide headers from the user. The user is responsible for making headers, such as by using make_headers.

schlamar · 2013-05-03T14:22:42Z

Anyone familiar with software copyright/licenses have insight on whether it's a problem to introduce separately licensed submodules into the main codebase? (dummyserver/proxy.py) Should we try to consolidate the licenses?

This is exactly the same thing than the vendored libraries in packages or I am missing something?!

I'm still seeing a bunch of # TODO's, is that intended?

The first TODO is proposing an optimization in a backward compatibility branch, this can be ignored/dropped.

Basic proxy authorization looks pretty simple (from urllib2):

    if user and password:
        user_pass = '%s:%s' % (unquote(user), unquote(password))
        creds = base64.b64encode(user_pass).strip()
        req.add_header('Proxy-authorization', 'Basic ' + creds)

But I guess we should have a proper way to test this?

kislyuk · 2013-05-05T13:55:31Z

@shazow Please please please can this be merged? This makes requests work with https proxies, and doesn't break anything that we could detect. But since no version of requests or urllib3 on pypi carries this patchset, we have to vendor requests+urllib3 with it applied.

shazow · 2013-05-05T19:28:33Z

urllib3/connectionpool.py

@@ -171,12 +177,20 @@ class HTTPConnectionPool(ConnectionPool, RequestMethods):
    :param headers:
        Headers to include with all requests, unless other headers are given
        explicitly.
+
+    :param proxy:


Since these params are "internal", should we prefix them with an underscore? _proxy and _proxy_headers.

Honestly I'm not a huge fan of having them here, but I can't think of a better solution right now.

shazow · 2013-05-05T19:41:58Z

Hey @kislyuk and fellow Andrey.

My apologies, this has been a very long-lived issue and has been a lot of work to get right. We're almost there, though. This branch looks very close but it may still be some time until it's merged and published.

If you'd like to help, more reviews of this code would be appreciated, and any additional tests you can add are always helpful too.

Alternatively, you can campaign for @kennethreitz to maintain a vendored version of urllib3 with this merged in, but that might be a hard sell. :)

@schlamar Thanks for all of your work so far! Could you also rebase this branch on the latest master when you have a chance?

schlamar · 2013-05-06T05:50:32Z

@shazow Done.

@stanvit You can commit the proposed changes to #139, then I'll cherry-pick them from there into this branch.

schlamar · 2013-07-19T06:58:01Z

Some failing tests on Windows, someone any idea: https://gist.github.com/schlamar/03b01c41bab64ed02bbf

schlamar · 2013-07-19T07:04:03Z

I have updated my automatic test script with the latest rebased versions of urllib3 and requests. Everyone with a proxy environment should give it a try: https://gist.github.com/schlamar/5080598

schlamar · 2013-07-19T07:52:10Z

Things to address (AFAIS). These are links to specific comments:

All discussions in the diff
HTTPS proxy support (issue #50) #139 (comment) Bug if SSL is not available.
Clean rebased version of https-proxy #170 (comment) TODO comments.
I think only cleaning them up is relevant here, proxy auth
can be addressed separately. The best would be to create
an issue for it so it won't be forgotten :)
Clean rebased version of https-proxy #170 (comment) Failing tests on Windows.

Edit: Ping @shazow Anything else?

Lukasa · 2013-07-19T08:18:21Z

I can try taking a look at some of the windows stuff this evening if no-one gets around to it before me.

t-8ch · 2013-07-19T14:15:47Z

The test is failing, because the response returned by the dummy_server is missing the Reason-Phrase.
(This is checked in tornado/simple_httpclient:_HTTPConnection:_on_headers())

Changing the status of the response returned from dummyserver/handlers.py:TestingApp:redirect() to include this phrase (303 See Other) fixes this.

See #205.

schlamar · 2013-07-19T15:07:15Z

Ah, I guess this is only an issue for Tornado >= 3. Should fix my test dependencies...

shazow · 2013-07-19T18:02:54Z

@schlamar Your list sounds about right. Added it as a check list to the bug description.

t-8ch · 2013-07-19T18:18:14Z

@shazow Last TODO is fixed. (failing test)

t-8ch · 2013-07-19T19:09:31Z

#208 fixes the bug about SSL not being available. (second TODO)

hjwp · 2013-07-22T11:11:16Z

Just +1'ing in order to track the issue. Sounds like this has been going for a long time, well done to everyone involved, keep your spirits up, and thanks for all the hard work!

…hem private

schlamar · 2013-07-31T18:46:49Z

New rebase against master.

@stanvit I revoked your write access because I'm back from vacation. Please send your changes as pull request against my proxy-rebase-fix branch. I often do history rewrites so this would be nasty if we two write in parallel to the same branch (I already had to clean up your merge commit).

schlamar · 2013-07-31T18:58:34Z

Updated the todo list.

@shazow Last two points are waiting for your answer :)

schlamar · 2013-07-31T19:00:20Z

@stanvit I stripped out this commit in favor of this one. I assume they have the same goal, right?

shazow · 2013-07-31T19:02:23Z

I'm satisfied. :) Is this it for everyone?

schlamar · 2013-07-31T19:06:41Z

Ship it 👍

shazow · 2013-07-31T19:06:57Z

urllib3/poolmanager.py

@@ -145,7 +149,10 @@ def urlopen(self, method, url, redirect=True, **kw):
        if 'headers' not in kw:
            kw['headers'] = self.headers

-        response = conn.urlopen(method, u.request_uri, **kw)
+        if self.proxy is not None and u.scheme == "http":


Can you add a comment explaining this condition?

Also would it be better to just set url = u.request_uri rather than fork the whole line?

shazow · 2013-07-31T19:09:44Z

Couple of minor style requests, otherwise ready to merge. :)

HTTPs proxy support.

shazow · 2013-08-01T17:35:07Z

You guys are amazing, thank you all. ✨

koobs · 2013-08-02T00:38:34Z

🏁 SHIP IT BABY! 🏁 - Great effort all

stanvit · 2013-08-02T01:39:48Z

Thank you everybody for pushing this!

JakeAustwick · 2013-08-13T03:30:13Z

Is it possible this isn't passing auth details along? After applying the above patch I am getting:
requests.packages.urllib3.exceptions.ProxyError: Cannot connect to proxy. Socket error: Tunnel connection failed: 407 Proxy Authentication Required.

Before the patch I was getting:
requests.exceptions.HTTPError: 501 Server Error: Not Implemented

schlamar · 2013-08-13T07:05:26Z

Is it possible this isn't passing auth details along?

Yes, proxy auth is still missing.

JakeAustwick · 2013-08-13T14:01:21Z

Is there anyway I can make this work, by passing in the headers manually or something? Or is it simply not supported right now?

schlamar · 2013-08-13T14:25:39Z

Is there anyway I can make this work, by passing in the headers manually or something?

This should work I guess :)

But please create a new ticket to request proxy auth support.

stanvit · 2013-08-14T05:12:49Z

Proxy authentication is working for me on the low level: the Proxy-Authorization header should be supplied in proxy_headers:

>>> from urllib3.poolmanager import ProxyManager
>>> goodauth = ProxyManager('http://localhost:3128', proxy_headers={'Proxy-Authorization': 'Basic dGVzdDp0ZXN0'})
>>> goodauth.request('GET', 'https://httpbin.org/get').status
200
>>> goodauth.request('GET', 'http://httpbin.org/get').status
200
>>> badauth = ProxyManager('http://localhost:3128', proxy_headers={'Proxy-Authorization': 'Basic garbage'})
>>> badauth.request('GET', 'https://httpbin.org/get').status
Traceback (most recent call last):
...
urllib3.exceptions.ProxyError: Cannot connect to proxy. Socket error: Tunnel connection failed: 407 Proxy Authentication Required.
>>> badauth.request('GET', 'http://httpbin.org/get').status
407

shazow · 2013-08-14T05:41:19Z

Would be awesome to get some examples like that into the docs, if anyone wants to do a PR. :)

This was referenced Apr 5, 2013

Merged against master stanvit/urllib3#5

Closed

HTTPS proxy support (issue #50) #139

Closed

Lukasa mentioned this pull request Apr 23, 2013

Issue of sending a HTTPS request use http proxy psf/requests#1318

Closed

shazow reviewed May 5, 2013
View reviewed changes

Lukasa mentioned this pull request May 15, 2013

Wrong results using proxies and HTTPS psf/requests#1359

Closed

astratto mentioned this pull request May 15, 2013

No proxy suppoert bgreenlee/sublime-github#31

Closed

schlamar mentioned this pull request Jul 19, 2013

SSL verification broken if using a proxy psf/requests#1209

Closed

t-8ch mentioned this pull request Jul 19, 2013

Add reason phrases to responses from dummyserver #205

Merged

stanvit and others added 5 commits July 31, 2013 20:41

ProxyManager PEP8 love

70628ce

Renamed proxy* arguments for HTTPConnectionPool constructor to mark t…

bedc00c

…hem private

Simplified ProxyServerThread.

72db03e

Moved all connection-related block from _new_conn() to a helper function

4d7f084

Removed irrelevant TODO comments

b9064e8

shazow reviewed Jul 31, 2013
View reviewed changes

Indentation fixes and some clean up.

c118546

shazow added a commit that referenced this pull request Aug 1, 2013

Merge pull request #170 from schlamar/proxy-rebase-fix

14b8945

HTTPs proxy support.

shazow merged commit 14b8945 into urllib3:master Aug 1, 2013

schlamar mentioned this pull request Aug 1, 2013

HTTPS proxy support psf/requests#1512

Closed

schlamar deleted the proxy-rebase-fix branch August 1, 2013 20:01

shazow added a commit that referenced this pull request Aug 1, 2013

Added #220 and #170 to CHANGES.

f6e23e3

JshWright mentioned this pull request Sep 20, 2013

Update urllib3 to b4391c1 psf/requests#1613

Closed

schlamar mentioned this pull request Jan 27, 2015

HTTPS requests fail through a proxy #50

Closed

Clean rebased version of https-proxy #170

Clean rebased version of https-proxy #170

Conversation

schlamar commented Apr 5, 2013

Lukasa commented Apr 23, 2013

shazow commented Apr 23, 2013

Anorov commented Apr 23, 2013

shazow commented Apr 24, 2013

schlamar commented May 3, 2013

kislyuk commented May 5, 2013

shazow May 5, 2013

Choose a reason for hiding this comment

shazow commented May 5, 2013

schlamar commented May 6, 2013

schlamar commented Jul 19, 2013

schlamar commented Jul 19, 2013

schlamar commented Jul 19, 2013

Lukasa commented Jul 19, 2013

t-8ch commented Jul 19, 2013

schlamar commented Jul 19, 2013

shazow commented Jul 19, 2013

t-8ch commented Jul 19, 2013

t-8ch commented Jul 19, 2013

hjwp commented Jul 22, 2013

schlamar commented Jul 31, 2013

schlamar commented Jul 31, 2013

schlamar commented Jul 31, 2013

shazow commented Jul 31, 2013

schlamar commented Jul 31, 2013

shazow Jul 31, 2013

Choose a reason for hiding this comment

shazow commented Jul 31, 2013

shazow commented Aug 1, 2013

koobs commented Aug 2, 2013

stanvit commented Aug 2, 2013

JakeAustwick commented Aug 13, 2013

schlamar commented Aug 13, 2013

JakeAustwick commented Aug 13, 2013

schlamar commented Aug 13, 2013

stanvit commented Aug 14, 2013

shazow commented Aug 14, 2013