Skip to content

Added ability to choose SSL version #109

Closed
wants to merge 4 commits into from

5 participants

@dandrzejewski

This is the same as a change that I did for the requests project - which
required this urllib3 change - so I thought it might be appropriate to
contribute the change to the upstream library as well.

@dandrzejewski dandrzejewski Added ability to choose SSL protocol version
This is the same as a change that I did for the requests project - which
required this urllib3 change - so I thought it might be appropriate to
contribute the change to the upstream library as well.
5557476
@sigmavirus24
Collaborator

If you're submitting this here, you need to add unit tests so @shazow can sleep comfortably with 100% test coverage.

@dandrzejewski

I will try to, although I'm not sure how I can verify that it's actually using the chosen SSL protocol version.

@dandrzejewski

It looks like there's no easy way to check on that, and there's also no easy way to make Tornado start up and only support certain SSL versions. However, if I force SSLv2, it throws an exception, so I'll submit two new tests: One that forces it to TLSv1 (checks for a 200 response), and one that forces it to SSLv2 (checks for an SSLError exception)

@sigmavirus24
Collaborator

Hm, @shazow might have some insight on writing tests for this.

@shazow shazow commented on the diff Oct 20, 2012
test/with_dummyserver/test_https.py
@@ -21,9 +22,21 @@ def setUp(self):
def test_simple(self):
r = self._pool.request('GET', '/specific_method',
- fields={'method': 'GET'})
+ fields={'method': 'GET'})
@shazow
Owner
shazow added a note Oct 20, 2012

Please keep indents (and other things) PEP8 style. :)

Whoops didn't catch that. I'll get it corrected!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@shazow shazow and 1 other commented on an outdated diff Oct 20, 2012
urllib3/connectionpool.py
@@ -76,6 +76,7 @@ class VerifiedHTTPSConnection(HTTPSConnection):
"""
cert_reqs = None
ca_certs = None
+ ssl_version = ssl.PROTOCOL_SSLv23
@shazow
Owner
shazow added a note Oct 20, 2012

This line will explode if ssl is None (see try/except header up top, not all platforms have Python compiled with SSL).

@shazow
Owner
shazow added a note Oct 20, 2012

Can we pass it in the same way we pass in cert_reqs and ca_certs?

I'm fine with assuming it's SSLv23 if ssl_version is None.

OK. I'll set ssl_version = None up top.

For the constructor, I'm assuming you'd rather I default ssl_version=None and then, in _new_conn(), check if self.ssl_version is None and set it there accordingly? That way, it'll be done after ssl has been checked for and SSLError raised.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@shazow shazow commented on the diff Oct 21, 2012
urllib3/connectionpool.py
@@ -76,6 +76,7 @@ class VerifiedHTTPSConnection(HTTPSConnection):
"""
cert_reqs = None
ca_certs = None
+ ssl_version = None
@shazow
Owner
shazow added a note Oct 21, 2012

Why are we tracking ssl_version separately both in HTTPSConnectionPool and VerifiedHTTPSConnection? Shouldn't it be one or the other?

Doesn't HTTPSConnectionPool, pass it into VerifiedHTTPSConnection?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@shazow
Owner
shazow commented Oct 21, 2012

Thanks for the fixes! A mention of the new ssl_version param in the docstring for HTTPSConnectionPool, or wherever we end up keeping, it would be great. :)

@dandrzejewski

Done.

This was referenced Nov 22, 2012
@shazow
Owner
shazow commented Dec 16, 2012

Hrm. I'm getting this on OSX using Py27:

........................F..........................................................
======================================================================
FAIL: test_set_ssl_version_to_sslv2 (test.with_dummyserver.test_https.TestHTTPS)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/shazow/projects/urllib3/test/with_dummyserver/test_https.py", line 38, 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
urllib3.connectionpool: DEBUG: "GET /specific_method?method=GET HTTP/1.1" 200 0
root: INFO: 200 GET /specific_method?method=GET (127.0.0.1) 0.78ms
--------------------- >> end captured logging << ---------------------
@shazow shazow commented on the diff Dec 16, 2012
test/with_dummyserver/test_https.py
self.assertEqual(r.status, 200, r.data)
+ def test_set_ssl_version_to_tlsv1(self):
+ self._pool.ssl_version = ssl.PROTOCOL_TLSv1
+ r = self._pool.request('GET', '/specific_method',
+ fields={'method': 'GET'})
+ self.assertEqual(r.status, 200, r.data)
+
+ def test_set_ssl_version_to_sslv2(self):
+ self._pool.ssl_version = ssl.PROTOCOL_SSLv2
+ with self.assertRaises(SSLError):
@shazow
Owner
shazow added a note Dec 16, 2012

Why should this raise?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Schwanksta

@shazow So is this issue taken care of yet? :)

@shazow
Owner
shazow commented Dec 19, 2012

@Schwanksta It has been merged into master. There might be minor changes before it's published, but it should be usable already.

@Schwanksta

Awesome, I thought so, but I wasn't sure because the ticket was still open. Thanks @shazow !

@shazow shazow closed this Dec 19, 2012
@kevinburke

Might be missing something but I can't find these commits in urllib3 master...

@shazow
Owner
shazow commented Jan 14, 2014

Bits and pieces are still there I think, but we've had several other ssl-related PRs which probably mutated much of the diff.

I think it's largely urllib3.util.resolve_ssl_version and such now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.