Added ability to choose SSL version #109

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
5 participants
@dandrzejewski
Contributor

dandrzejewski commented Oct 19, 2012

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.

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.
@sigmavirus24

This comment has been minimized.

Show comment
Hide comment
@sigmavirus24

sigmavirus24 Oct 19, 2012

Member

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

Member

sigmavirus24 commented Oct 19, 2012

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

@dandrzejewski

This comment has been minimized.

Show comment
Hide comment
@dandrzejewski

dandrzejewski Oct 19, 2012

Contributor

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

Contributor

dandrzejewski commented Oct 19, 2012

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

@dandrzejewski

This comment has been minimized.

Show comment
Hide comment
@dandrzejewski

dandrzejewski Oct 19, 2012

Contributor

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)

Contributor

dandrzejewski commented Oct 19, 2012

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

This comment has been minimized.

Show comment
Hide comment
@sigmavirus24

sigmavirus24 Oct 20, 2012

Member

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

Member

sigmavirus24 commented Oct 20, 2012

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

@@ -21,9 +22,21 @@ def setUp(self):
def test_simple(self):
r = self._pool.request('GET', '/specific_method',
- fields={'method': 'GET'})
+ fields={'method': 'GET'})

This comment has been minimized.

@shazow

shazow Oct 20, 2012

Collaborator

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

@shazow

shazow Oct 20, 2012

Collaborator

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

This comment has been minimized.

@dandrzejewski

dandrzejewski Oct 20, 2012

Contributor

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

@dandrzejewski

dandrzejewski Oct 20, 2012

Contributor

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

urllib3/connectionpool.py
@@ -76,6 +76,7 @@ class VerifiedHTTPSConnection(HTTPSConnection):
"""
cert_reqs = None
ca_certs = None
+ ssl_version = ssl.PROTOCOL_SSLv23

This comment has been minimized.

@shazow

shazow Oct 20, 2012

Collaborator

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

@shazow

shazow Oct 20, 2012

Collaborator

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

This comment has been minimized.

@shazow

shazow Oct 20, 2012

Collaborator

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.

@shazow

shazow Oct 20, 2012

Collaborator

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.

This comment has been minimized.

@dandrzejewski

dandrzejewski Oct 20, 2012

Contributor

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.

@dandrzejewski

dandrzejewski Oct 20, 2012

Contributor

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.

@@ -76,6 +76,7 @@ class VerifiedHTTPSConnection(HTTPSConnection):
"""
cert_reqs = None
ca_certs = None
+ ssl_version = None

This comment has been minimized.

@shazow

shazow Oct 21, 2012

Collaborator

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

@shazow

shazow Oct 21, 2012

Collaborator

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

This comment has been minimized.

@dandrzejewski

dandrzejewski Oct 22, 2012

Contributor

Doesn't HTTPSConnectionPool, pass it into VerifiedHTTPSConnection?

@dandrzejewski

dandrzejewski Oct 22, 2012

Contributor

Doesn't HTTPSConnectionPool, pass it into VerifiedHTTPSConnection?

@shazow

This comment has been minimized.

Show comment
Hide comment
@shazow

shazow Oct 21, 2012

Collaborator

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. :)

Collaborator

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

This comment has been minimized.

Show comment
Hide comment
@dandrzejewski

dandrzejewski Oct 22, 2012

Contributor

Done.

Contributor

dandrzejewski commented Oct 22, 2012

Done.

@t-8ch t-8ch referenced this pull request Nov 22, 2012

Merged

add support for SNI #89

@shazow

This comment has been minimized.

Show comment
Hide comment
@shazow

shazow Dec 16, 2012

Collaborator

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 << ---------------------
Collaborator

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 << ---------------------
+
+ def test_set_ssl_version_to_sslv2(self):
+ self._pool.ssl_version = ssl.PROTOCOL_SSLv2
+ with self.assertRaises(SSLError):

This comment has been minimized.

@shazow

shazow Dec 16, 2012

Collaborator

Why should this raise?

@shazow

shazow Dec 16, 2012

Collaborator

Why should this raise?

@Schwanksta

This comment has been minimized.

Show comment
Hide comment
@Schwanksta

Schwanksta Dec 19, 2012

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

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

@shazow

This comment has been minimized.

Show comment
Hide comment
@shazow

shazow Dec 19, 2012

Collaborator

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

Collaborator

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

This comment has been minimized.

Show comment
Hide comment
@Schwanksta

Schwanksta Dec 19, 2012

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

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

@kevinburke

This comment has been minimized.

Show comment
Hide comment
@kevinburke

kevinburke Jan 14, 2014

Contributor

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

Contributor

kevinburke commented Jan 14, 2014

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

@shazow

This comment has been minimized.

Show comment
Hide comment
@shazow

shazow Jan 14, 2014

Collaborator

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.

Collaborator

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