HTTPS proxy support (issue #50) #139

stanvit · 2013-01-09T02:52:21Z

This Pull Request resolves #50 adding HTTPS (CONNECT) proxy support to ProxyManager with minimal API and code changes.

A short summary of the changes:

ProxyManager is a subclass of PoolManager now and its logic has been changed completely, though its constructor still accepts HTTPConnectionPool as the main argument so it's backwards compatible
Tiny change to VerifiedHTTPSConnection.connect() that introduces CONNECT tunneling support (copied from HTTPConnection.connect() code)
HTTPSConnectionPool._new_conn() has been changed, but its logic for non-proxied connections stays the same
HTTPSConnectionPool._new_conn() actually establishes the connection for proxied requests by calling HTTPSConnection.connect(), otherwise Host: header is set improperly due to httplib logic. As stated above, the behavior for non-proxied requests hasn't changed
tornado-proxy has been added for tests
TornadoServerThread has been changed a bit - now it's possible to run multiple server threads in a single test case
Added proxy tests
Docs updated

All tests run fine with Python 2.6, 2.7 and 3.2 on Mac OS 10.7.5, except for sporadic timeouts and SSL errors that occur against the unpatched library as well (and should be fixed separately). The code doesn't introduce any new regressions.

Possible TODOs and improvements:

Proxy authentication (though that could be another libraries' responsibility, like requests)
When accepting proxy_pool as the first argument for ProxyManager, copy all pool properties and use them for other pool instances generation or even use it as the pool for HTTP proxy requests
Usage of HTTPConnectionPool and, especially, HTTPSConnectionPool against proxy isn't really clear and requires some external logic - much like ProxyManager.connection_from_host()

…st: header issue.

shazow · 2013-01-09T04:42:25Z

This looks very interesting, thank you!

I'm going to try and take a closer look at it over the next couple of weeks. Please bug me if I'm taking too long. Things have been busy lately. :)

shazow · 2013-01-19T19:02:57Z

@wolever is going to take a closer look at this for me. :)

wolever · 2013-01-19T19:56:57Z

First, as of 4a5f5f7, there is one failing test:

$ git show
commit 4a5f5f765e857e4378d1eb8678de8e4c37cb61d4
Author: Stanislav Vitkovskiy <stas.vitkovsky@gmail.com>
Date:   Wed Jan 9 12:33:47 2013 +1100

    Added old ProxyManager API compatibility
…
$ nosetests
........................F..................................................................
======================================================================
FAIL: test_set_ssl_version_to_sslv2 (test.with_dummyserver.test_https.TestHTTPS)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/wolever/code/urllib3/test/with_dummyserver/test_https.py", line 39, in test_set_ssl_version_to_sslv2
    fields={'method': 'GET'})
AssertionError: SSLError not raised
-------------------- >> begin captured logging << --------------------
urllib3.connectionpool: INFO: Starting new HTTPS connection (1): localhost
root: INFO: 200 GET /specific_method?method=GET (::1) 0.40ms
urllib3.connectionpool: DEBUG: "GET /specific_method?method=GET HTTP/1.1" 200 0
--------------------- >> end captured logging << ---------------------

Name                     Stmts   Miss  Cover   Missing
------------------------------------------------------
urllib3                     14      0   100%   
urllib3._collections        43      0   100%   
urllib3.connectionpool     182      2    99%   575-576
urllib3.contrib              0      0   100%   
urllib3.exceptions          32      0   100%   
urllib3.filepost            34      0   100%   
urllib3.poolmanager         73      1    99%   176
urllib3.request             25      0   100%   
urllib3.response            74      0   100%   
urllib3.util                86      5    94%   258-263
------------------------------------------------------
TOTAL                      563      8    99%   
----------------------------------------------------------------------
Ran 91 tests in 1.061s

FAILED (failures=1)
$ git checkout master
$ nosetests
…
Ran 71 tests in 0.775s

OK

wolever · 2013-01-20T06:40:43Z

urllib3/connectionpool.py

@@ -96,6 +96,11 @@ def connect(self):
        # Add certificate verification
        sock = socket.create_connection((self.host, self.port), self.timeout)

+        if self._tunnel_host:
+            self.sock = sock
+            self._tunnel() # calls self._set_hostport(), so self.host is


Please move these "inline" comments above:

# calls self._set_hostport() … # … self._tunnel()

Also, sentence-case please. :)

# Calls `self._set_hostport()`, ...

wolever · 2013-01-20T07:33:35Z

One bug: it looks like the ProxyManager fails when no port is specified in the URL:

In [14]: proxy = urllib3.ProxyManager('http://localhost:80/')

In [15]: proxy.request("GET", "https://google.ca")
Out[15]: <urllib3.response.HTTPResponse at 0x10a3fce10>

In [16]: proxy = urllib3.ProxyManager('http://localhost/')

In [17]: proxy.request("GET", "https://google.ca")
…
MaxRetryError: HTTPSConnectionPool(host='google.ca', port=443): Max retries exceeded with url: / (Caused by <class 'socket.error'>: [Errno 61] Connection refused)

Anorov · 2013-03-15T09:00:00Z

Tiny bug:

This actually might actually exist in master too and might not have been introduced by this branch, but either way this would be a good place to fix it.

If you comment out import ssl at the top of connectionpool.py, connection to HTTPS websites via a proxy does not work and raises a confusing exception. Regular connections to HTTPS websites work without a proxy, even if ssl is not imported. I'm not really sure who would be using urllib3/requests without the ssl module, but since it is supported, it should be accounted for.

Easiest fix, in my opinion: change line 560 in connectionpool.py from if not HTTPSConnection or HTTPSConnection is object: to if HTTPSConnection is object or self.proxy: so the appropriate exception can be raised. The if not HTTPSConnection seems like a code relic and can be removed, I think, since HTTPSConnection will never coerce to False and would instead always be set to object.

schlamar · 2013-03-25T12:42:24Z

@stanvit If you do a rebase the Travis build will be triggered.

stanvit · 2013-03-30T06:58:47Z

@schlamar, I probably will need some assistance with rebasing, I couldn't make it properly. Every change seems to be applied twice (due the previous rebase, I guess I should have squashed them) and it will probably result in doubling all commits from https-proxy branch again.

schlamar · 2013-04-01T12:18:15Z

Mhh, that is strange. Could it be that you did the first rebase against upstream/master and the second against origin/master? Usually a second rebase against master should be fine. However, I'm no git expert...

schlamar · 2013-04-05T08:02:02Z

I merged against master: stanvit#5

schlamar · 2013-04-05T09:49:05Z

Rebased version is here: #170
Not sure if you can force this back into this branch, so I made a new PR. If you can clean up this PR, I will close #170.

schlamar · 2013-04-05T12:11:46Z

How I did this: I cherry-picked your first rebase to the master revision at that time (4ff6c97) and then did a clean rebase against master:

git checkout -b proxy-rebase-fix 4ff6c97
git cherry-pick f640206^..74444e9
git cherry-pick 6c19926
git cherry-pick 7d2ca06^..1aab973
git cherry-pick a54f794^..6c46167
git rebase master  
# resolve conflicts
git cherry-pick 5d987ce

shazow · 2013-04-05T14:15:47Z

Thank you all for continuing to hammer this out. :)

stanvit · 2013-04-08T10:02:02Z

@schlamar , thank you very much, I'll look at that tomorrow, I was pretty busy lately.

schlamar · 2013-04-12T06:41:13Z

@shazow @wolever What needs to be done for final approval? Can't wait to see this finally in master and requests :)

schlamar · 2013-04-12T06:42:09Z

@Anorov Pinging you, too. Any further suggestions? (see #170).

Anorov · 2013-04-13T03:46:36Z

Everything looks good, assuming all tests pass after the recent SSL changes in master. Also eagerly awaiting the approval of this, so I can make my official pull for SOCKS support on top of this.

schlamar · 2013-04-13T11:25:19Z

@Anorov A clean rebased version of this PR with Travis passing lives at #170.

williamsjj · 2013-05-06T20:16:24Z

This would be great to get merged in if it's passing tests now.

sigmavirus24 · 2013-07-19T03:25:48Z

Bump

shazow · 2013-07-19T05:22:23Z

Any brave champions want to shepherd this poor issue from the wilderness?

schlamar · 2013-07-19T05:59:38Z

@shazow You made a few comments at #170. Which of them have to be addressed and which one are optional? Some are really hard to deal with without response from @stanvit

stanvit · 2013-07-19T06:10:44Z

I'm sorry I haven't responded for a long time, been really busy with other things. I'll do my best to respond to questions asked by @shazow and others in #170. I can dedicate my time on this on Monday/Tuesday.

koobs · 2013-07-19T06:16:52Z

+1 on getting this merged as soon as possible.

@shazow What it looks like your keen contributors need is your leadership and a clear list of exactly what is left to be addressed so that everyone who's interested can swarm on getting it finished and finally start enjoying it.

Let's not let the recent activity and interest wane again. Clear goals!

GO GO GO!

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

@stanvit Glad to here from you again. I added you as collaborator to my urllib3 fork, so you can directly push to #170. I think it's best to focus on the new PR and close this one. I'll be on vacation till 1st August, so you can take the lead on this.

stanvit · 2013-07-19T07:24:45Z

Thank you, @shlamar . Could you please list the goals that, in your opinion, will lead to the PR acceptance?

schlamar · 2013-07-19T07:55:07Z

Added a list to #170, no guarantee for completeness though :)

Lukasa · 2013-07-19T08:17:44Z

A recommendation: can we keep the conversation in either this PR or the rebased one in #170? The conversation is difficult to follow when it's in two places. =)

schlamar · 2013-07-19T08:38:28Z

👍 This is exactly what I wanted to say in my previous comment (#139 (comment)).

shazow · 2013-07-19T17:56:52Z

Closing this PR to avoid confusion.

stanvit added 12 commits January 2, 2013 15:19

At least, HTTP(S) proxy support works now...

7722bf4

Fixed proxy test

15e3681

Inplemented CB's suggestions regarding code clarity. Fixed proxied Ho…

fd179cf

…st: header issue.

Fixed proxymanager headers logic

61c1ab7

Added proxymanager tests with tornado-based proxy

64e57a6

Fixed self.headers involuntary change

44c0819

Improved readability a bit

8be8d04

Created dummyserver tests for ProxyManager

adc17f3

Added docs

7dd3e5c

Python 2.6 tests compatibility fixed

2f3f0c5

Tests are python 3.2 compatible now

c9d0393

Added old ProxyManager API compatibility

4a5f5f7

wolever reviewed Jan 20, 2013
View reviewed changes

Remove unncecessary proxy code from PoolManager

6c46167

schlamar mentioned this pull request Apr 5, 2013

Clean rebased version of https-proxy #170

Merged

11 tasks

Lukasa mentioned this pull request May 15, 2013

Wrong results using proxies and HTTPS psf/requests#1359

Closed

shazow closed this Jul 19, 2013

phuslu mentioned this pull request Feb 23, 2017

allow http/https requests through https proxy #1121

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HTTPS proxy support (issue #50) #139

HTTPS proxy support (issue #50) #139

stanvit commented Jan 9, 2013

shazow commented Jan 9, 2013

shazow commented Jan 19, 2013

wolever commented Jan 19, 2013

wolever Jan 20, 2013

shazow Jan 20, 2013

wolever commented Jan 20, 2013

Anorov commented Mar 15, 2013

schlamar commented Mar 25, 2013

stanvit commented Mar 30, 2013

schlamar commented Apr 1, 2013

schlamar commented Apr 5, 2013

schlamar commented Apr 5, 2013

schlamar commented Apr 5, 2013

shazow commented Apr 5, 2013

stanvit commented Apr 8, 2013

schlamar commented Apr 12, 2013

schlamar commented Apr 12, 2013

Anorov commented Apr 13, 2013

schlamar commented Apr 13, 2013

williamsjj commented May 6, 2013

sigmavirus24 commented Jul 19, 2013

shazow commented Jul 19, 2013

schlamar commented Jul 19, 2013

stanvit commented Jul 19, 2013

koobs commented Jul 19, 2013

schlamar commented Jul 19, 2013

stanvit commented Jul 19, 2013

schlamar commented Jul 19, 2013

Lukasa commented Jul 19, 2013

schlamar commented Jul 19, 2013

shazow commented Jul 19, 2013

HTTPS proxy support (issue #50) #139

HTTPS proxy support (issue #50) #139

Conversation

stanvit commented Jan 9, 2013

shazow commented Jan 9, 2013

shazow commented Jan 19, 2013

wolever commented Jan 19, 2013

wolever Jan 20, 2013

Choose a reason for hiding this comment

shazow Jan 20, 2013

Choose a reason for hiding this comment

wolever commented Jan 20, 2013

Anorov commented Mar 15, 2013

schlamar commented Mar 25, 2013

stanvit commented Mar 30, 2013

schlamar commented Apr 1, 2013

schlamar commented Apr 5, 2013

schlamar commented Apr 5, 2013

schlamar commented Apr 5, 2013

shazow commented Apr 5, 2013

stanvit commented Apr 8, 2013

schlamar commented Apr 12, 2013

schlamar commented Apr 12, 2013

Anorov commented Apr 13, 2013

schlamar commented Apr 13, 2013

williamsjj commented May 6, 2013

sigmavirus24 commented Jul 19, 2013

shazow commented Jul 19, 2013

schlamar commented Jul 19, 2013

stanvit commented Jul 19, 2013

koobs commented Jul 19, 2013

schlamar commented Jul 19, 2013

stanvit commented Jul 19, 2013

schlamar commented Jul 19, 2013

Lukasa commented Jul 19, 2013

schlamar commented Jul 19, 2013

shazow commented Jul 19, 2013