Add openSUSE certificate support #629

Merged
merged 1 commit into from Jun 29, 2012

Conversation

Projects
None yet
4 participants
@saschpe
Contributor

saschpe commented May 23, 2012

openSUSE ships Mozilla CA certificates in a directory (/etc/ssl/certs) and for compatibility as a bundled pem (/etc/ssl/ca-bundle.pem), the first is preferred though. This patch adds support for both, i.e. if a path is found in POSSIBLE_CA_BUNDLE_PATHS, it checks whether the ssl module supports it. Otherwise, the pem file is used (listed after '/etc/ssl/certs' in POSSIBLE_CA_BUNDLE_PATHS).

@travisbot

This comment has been minimized.

Show comment Hide comment
@travisbot

travisbot May 23, 2012

This pull request passes (merged b22088fd into de1637c).

This pull request passes (merged b22088fd into de1637c).

@slingamn

This comment has been minimized.

Show comment Hide comment
@slingamn

slingamn May 23, 2012

Contributor

Looks good to me. Cool!

Contributor

slingamn commented May 23, 2012

Looks good to me. Cool!

@slingamn

This comment has been minimized.

Show comment Hide comment
@slingamn

slingamn May 23, 2012

Contributor

Actually is it possible to do this using the public ssl API, e.g., ssl.wrap_socket instead of _ssl.sslwrap?

Contributor

slingamn commented May 23, 2012

Actually is it possible to do this using the public ssl API, e.g., ssl.wrap_socket instead of _ssl.sslwrap?

@saschpe

This comment has been minimized.

Show comment Hide comment
@saschpe

saschpe May 24, 2012

Contributor

ssl.wrap_socket expects 'keyfile' and 'certfile', you can't pass a directory. AFAICS, the situation will improve with Python-3.3, where the ssl module will pick OS defaults by itself: http://bugs.python.org/issue14780

Contributor

saschpe commented May 24, 2012

ssl.wrap_socket expects 'keyfile' and 'certfile', you can't pass a directory. AFAICS, the situation will improve with Python-3.3, where the ssl module will pick OS defaults by itself: http://bugs.python.org/issue14780

@slingamn

This comment has been minimized.

Show comment Hide comment
@slingamn

slingamn May 24, 2012

Contributor

Sounds good to me.

Contributor

slingamn commented May 24, 2012

Sounds good to me.

@slingamn

This comment has been minimized.

Show comment Hide comment
@slingamn

slingamn May 26, 2012

Contributor

Actually, wait, I think this is not going to work, because urllib3 uses ssl.wrap_socket, not _ssl.sslwrap. So if wrap_socket does not understand directories, then our code will not either.

See VerifiedHTTPSConnection in this file: requests/packages/urllib3/connectionpool.py.

Contributor

slingamn commented May 26, 2012

Actually, wait, I think this is not going to work, because urllib3 uses ssl.wrap_socket, not _ssl.sslwrap. So if wrap_socket does not understand directories, then our code will not either.

See VerifiedHTTPSConnection in this file: requests/packages/urllib3/connectionpool.py.

@saschpe

This comment has been minimized.

Show comment Hide comment
@saschpe

saschpe May 30, 2012

Contributor

So be it, then the fallback file is used

Contributor

saschpe commented May 30, 2012

So be it, then the fallback file is used

@slingamn

This comment has been minimized.

Show comment Hide comment
@slingamn

slingamn May 31, 2012

Contributor

Well, it seems like there are two possibilities:

  1. ssl.wrap_socket supports (without documentation) passing a directory instead of a file as its ca_certs parameter. In that case, this patch is correct, except that it should use ssl.wrap_socket instead of _ssl.sslwrap (since that is a more accurate test of whether Requests will work with CA certificate directories).
  2. ssl.wrap_socket does not support this, despite the backing function _ssl.sslwrap supporting it. In that case, there is no hope of Requests ever being able to use a certificate directory, and this patch should just be the one-line addition of /etc/ssl/ca-bundle.pem as a possible CA certificate bundle file.

Have you observed Requests to work correctly with /etc/ssl/certs on your system?

Thanks for your patience by the way :-)

Contributor

slingamn commented May 31, 2012

Well, it seems like there are two possibilities:

  1. ssl.wrap_socket supports (without documentation) passing a directory instead of a file as its ca_certs parameter. In that case, this patch is correct, except that it should use ssl.wrap_socket instead of _ssl.sslwrap (since that is a more accurate test of whether Requests will work with CA certificate directories).
  2. ssl.wrap_socket does not support this, despite the backing function _ssl.sslwrap supporting it. In that case, there is no hope of Requests ever being able to use a certificate directory, and this patch should just be the one-line addition of /etc/ssl/ca-bundle.pem as a possible CA certificate bundle file.

Have you observed Requests to work correctly with /etc/ssl/certs on your system?

Thanks for your patience by the way :-)

@saschpe

This comment has been minimized.

Show comment Hide comment
@saschpe

saschpe Jun 6, 2012

Contributor

Hi, sorry for the delay, I did the following to test it:

>>> import ssl,socket,pprint
>>> ssl_sock = ssl.wrap_socket(s,cert_reqs=ssl.CERT_REQUIRED,ca_certs='/etc/ssl/certs')
>>> ssl_sock.connect(('www.verisign.com', 443))
>>> print repr(ssl_sock.getpeername())
('69.58.181.89', 443)
>>> print ssl_sock.cipher()
('DHE-RSA-AES256-SHA', 'TLSv1/SSLv3', 256)
>>> print pprint.pformat(ssl_sock.getpeercert())
{'notAfter': 'May 17 23:59:59 2014 GMT',
 'subject': ((('1.3.6.1.4.1.311.60.2.1.3', u'US'),),
             (('1.3.6.1.4.1.311.60.2.1.2', u'Delaware'),),
             (('businessCategory', u'Private Organization'),),
             (('serialNumber', u'2158113'),),
             (('countryName', u'US'),),
             (('postalCode', u'94043'),),
             (('stateOrProvinceName', u'California'),),
             (('localityName', u'Mountain View'),),
             (('streetAddress', u'350 Ellis Street'),),
             (('organizationName', u'Symantec Corporation'),),
             (('organizationalUnitName', u'Infrastructure Operations'),),
             (('commonName', u'www.verisign.com'),)),
 'subjectAltName': (('DNS', 'verisign.com'),
                    ('DNS', 'www.verisign.net'),
                    ('DNS', 'verisign.net'),
                    ('DNS', 'www.verisign.mobi'),
                    ('DNS', 'verisign.mobi'),
                    ('DNS', 'www.verisign.eu'),
                    ('DNS', 'verisign.eu'),
                    ('DNS', 'www.verisign.com'))}
>>> ssl_sock.write("""GET / HTTP/1.0\rHOST: www.verisign.com\r\n\r\n""")
41
>>> data = ssl_sock.read()
>>> data
'HTTP/1.1 301 Moved Permanently\r\nDate: Wed, 06 Jun 2012 14:51:17 GMT\r\nServer: Apache\r\nLocation: https://www.verisign.com/\r\nCac
he-Control: max-age=2592000\r\nExpires: Fri, 06 Jul 2012 14:51:17 GMT\r\nVary: Accept-Encoding\r\nContent-Length: 233\r\nConnection: c
lose\r\nContent-Type: text/html; charset=iso-8859-1\r\n\r\n'
>>> ssl_sock.ca_certs
'/etc/ssl/certs'
>>> ssl_sock.close()

So, I assume "1."

Changing the check to ssl.wrap_socket won't change much, ssl.SSLSocket (returned by ssl.wrap_socket) uses _ssl.sslwrap internally (have a look at /usr/lib64/python2.7/ssl.py).

Finally, requests.models.Request has "conn.ca_certs = cert_loc = DEFAULT_CA_BUNDLE_PATH", thus passing a path works for me ;-)

Contributor

saschpe commented Jun 6, 2012

Hi, sorry for the delay, I did the following to test it:

>>> import ssl,socket,pprint
>>> ssl_sock = ssl.wrap_socket(s,cert_reqs=ssl.CERT_REQUIRED,ca_certs='/etc/ssl/certs')
>>> ssl_sock.connect(('www.verisign.com', 443))
>>> print repr(ssl_sock.getpeername())
('69.58.181.89', 443)
>>> print ssl_sock.cipher()
('DHE-RSA-AES256-SHA', 'TLSv1/SSLv3', 256)
>>> print pprint.pformat(ssl_sock.getpeercert())
{'notAfter': 'May 17 23:59:59 2014 GMT',
 'subject': ((('1.3.6.1.4.1.311.60.2.1.3', u'US'),),
             (('1.3.6.1.4.1.311.60.2.1.2', u'Delaware'),),
             (('businessCategory', u'Private Organization'),),
             (('serialNumber', u'2158113'),),
             (('countryName', u'US'),),
             (('postalCode', u'94043'),),
             (('stateOrProvinceName', u'California'),),
             (('localityName', u'Mountain View'),),
             (('streetAddress', u'350 Ellis Street'),),
             (('organizationName', u'Symantec Corporation'),),
             (('organizationalUnitName', u'Infrastructure Operations'),),
             (('commonName', u'www.verisign.com'),)),
 'subjectAltName': (('DNS', 'verisign.com'),
                    ('DNS', 'www.verisign.net'),
                    ('DNS', 'verisign.net'),
                    ('DNS', 'www.verisign.mobi'),
                    ('DNS', 'verisign.mobi'),
                    ('DNS', 'www.verisign.eu'),
                    ('DNS', 'verisign.eu'),
                    ('DNS', 'www.verisign.com'))}
>>> ssl_sock.write("""GET / HTTP/1.0\rHOST: www.verisign.com\r\n\r\n""")
41
>>> data = ssl_sock.read()
>>> data
'HTTP/1.1 301 Moved Permanently\r\nDate: Wed, 06 Jun 2012 14:51:17 GMT\r\nServer: Apache\r\nLocation: https://www.verisign.com/\r\nCac
he-Control: max-age=2592000\r\nExpires: Fri, 06 Jul 2012 14:51:17 GMT\r\nVary: Accept-Encoding\r\nContent-Length: 233\r\nConnection: c
lose\r\nContent-Type: text/html; charset=iso-8859-1\r\n\r\n'
>>> ssl_sock.ca_certs
'/etc/ssl/certs'
>>> ssl_sock.close()

So, I assume "1."

Changing the check to ssl.wrap_socket won't change much, ssl.SSLSocket (returned by ssl.wrap_socket) uses _ssl.sslwrap internally (have a look at /usr/lib64/python2.7/ssl.py).

Finally, requests.models.Request has "conn.ca_certs = cert_loc = DEFAULT_CA_BUNDLE_PATH", thus passing a path works for me ;-)

@slingamn

This comment has been minimized.

Show comment Hide comment
@slingamn

slingamn Jun 10, 2012

Contributor

Excellent, I believe that 1 is in fact the case.

I would greatly prefer it if the patch could use ssl.wrap_socket instead of _ssl.sslwrap, though, for the following reasons:

  1. The behavior that Requests will actually take advantage of is the ability of ssl.wrap_socket to use a directory as its ca_certs, so it is a more direct test of the required functionality
  2. Since _ssl.sslwrap is not part of the public API, it's possible (although unlikely) that it might be renamed or moved in future. It seems much better to use undocumented behavior of the public API than to use an (also undocumented) private API.

The intended meaning of DEFAULT_CA_BUNDLE_PATH was "fully qualified path", not "a directory as opposed to a file" :-)

Thanks again! Sorry about the delay, I think I missed the GitHub notification.

Contributor

slingamn commented Jun 10, 2012

Excellent, I believe that 1 is in fact the case.

I would greatly prefer it if the patch could use ssl.wrap_socket instead of _ssl.sslwrap, though, for the following reasons:

  1. The behavior that Requests will actually take advantage of is the ability of ssl.wrap_socket to use a directory as its ca_certs, so it is a more direct test of the required functionality
  2. Since _ssl.sslwrap is not part of the public API, it's possible (although unlikely) that it might be renamed or moved in future. It seems much better to use undocumented behavior of the public API than to use an (also undocumented) private API.

The intended meaning of DEFAULT_CA_BUNDLE_PATH was "fully qualified path", not "a directory as opposed to a file" :-)

Thanks again! Sorry about the delay, I think I missed the GitHub notification.

@saschpe

This comment has been minimized.

Show comment Hide comment
@saschpe

saschpe Jun 11, 2012

Contributor

I agree that using ssl.wrap_socket would be much better. However, ssl.wrap_socket does not complain about invalid parameters (by throwing an SSLError exception like _ssl.sslwrap), only ssl.SSLSocket.connect() does. So, doing the check with ssl.wrap_socket would actually include a real request, e.g. like this:

--- a/requests/utils.py
+++ b/requests/utils.py
@@ -15,7 +15,6 @@ import os
 import re
 import socket
 import ssl
-import _ssl
 import zlib
 from netrc import netrc, NetrcParseError

@@ -57,7 +56,9 @@ def get_os_ca_bundle_path():
             if os.path.isdir(path):
                 try:
                     # Current candidate is a directory, check if SSL module supports that
-                    _ssl.sslwrap(socket.socket()._sock, False, None, None, ssl.CERT_REQUIRED, ssl.PROTOCOL_SSLv23, path, None)
+                    ssl.wrap_socket(socket.socket(),cert_reqs=ssl.CERT_REQUIRED,ca_certs=path)
+                    ssl_sock.connect(('www.verisign.com', 443))
+
                     return path
                 except:
                     pass # No support, let's check the next candidate

I don't think this is really desired but I don't see any other way to it ATM. What do you think?

Contributor

saschpe commented Jun 11, 2012

I agree that using ssl.wrap_socket would be much better. However, ssl.wrap_socket does not complain about invalid parameters (by throwing an SSLError exception like _ssl.sslwrap), only ssl.SSLSocket.connect() does. So, doing the check with ssl.wrap_socket would actually include a real request, e.g. like this:

--- a/requests/utils.py
+++ b/requests/utils.py
@@ -15,7 +15,6 @@ import os
 import re
 import socket
 import ssl
-import _ssl
 import zlib
 from netrc import netrc, NetrcParseError

@@ -57,7 +56,9 @@ def get_os_ca_bundle_path():
             if os.path.isdir(path):
                 try:
                     # Current candidate is a directory, check if SSL module supports that
-                    _ssl.sslwrap(socket.socket()._sock, False, None, None, ssl.CERT_REQUIRED, ssl.PROTOCOL_SSLv23, path, None)
+                    ssl.wrap_socket(socket.socket(),cert_reqs=ssl.CERT_REQUIRED,ca_certs=path)
+                    ssl_sock.connect(('www.verisign.com', 443))
+
                     return path
                 except:
                     pass # No support, let's check the next candidate

I don't think this is really desired but I don't see any other way to it ATM. What do you think?

@slingamn

This comment has been minimized.

Show comment Hide comment
@slingamn

slingamn Jun 15, 2012

Contributor

I'm sorry I'm such a pedant, but the use of _ssl still makes me uncomfortable. If you're continuing to ship /etc/ssl/ca-bundle.pem for compatibility, could we just use that?

Contributor

slingamn commented Jun 15, 2012

I'm sorry I'm such a pedant, but the use of _ssl still makes me uncomfortable. If you're continuing to ship /etc/ssl/ca-bundle.pem for compatibility, could we just use that?

@saschpe

This comment has been minimized.

Show comment Hide comment
@saschpe

saschpe Jun 15, 2012

Contributor

Yes, that would be an options for the time being and in order to get this solved, I'll provide a stripped down patch, deal? But this may become an issue again, as /etc/ssl/ca-bundle.pem is long time deprecated ;-)

Contributor

saschpe commented Jun 15, 2012

Yes, that would be an options for the time being and in order to get this solved, I'll provide a stripped down patch, deal? But this may become an issue again, as /etc/ssl/ca-bundle.pem is long time deprecated ;-)

@slingamn

This comment has been minimized.

Show comment Hide comment
@slingamn

slingamn Jun 15, 2012

Contributor

Deal.

Contributor

slingamn commented Jun 15, 2012

Deal.

@saschpe

This comment has been minimized.

Show comment Hide comment
@saschpe

saschpe Jun 15, 2012

Contributor

There you go, I updated the commit

Contributor

saschpe commented Jun 15, 2012

There you go, I updated the commit

@travisbot

This comment has been minimized.

Show comment Hide comment
@travisbot

travisbot Jun 15, 2012

This pull request fails (merged 14e22d14 into 08ac989).

This pull request fails (merged 14e22d14 into 08ac989).

@travisbot

This comment has been minimized.

Show comment Hide comment
@travisbot

travisbot Jun 15, 2012

This pull request fails (merged 66ef888 into 08ac989).

This pull request fails (merged 66ef888 into 08ac989).

@saschpe

This comment has been minimized.

Show comment Hide comment
@saschpe

saschpe Jun 15, 2012

Contributor

travisbot is crap

Contributor

saschpe commented Jun 15, 2012

travisbot is crap

@kennethreitz

This comment has been minimized.

Show comment Hide comment
@kennethreitz

kennethreitz Jun 15, 2012

Owner

Travisbot is wonderful, my tests are broken :)

Owner

kennethreitz commented Jun 15, 2012

Travisbot is wonderful, my tests are broken :)

@travisbot

This comment has been minimized.

Show comment Hide comment
@travisbot

travisbot Jun 29, 2012

This pull request passes (merged 66ef888 into 08ac989).

This pull request passes (merged 66ef888 into 08ac989).

@kennethreitz

This comment has been minimized.

Show comment Hide comment
@kennethreitz

kennethreitz Jun 29, 2012

Owner

Thanks!

Owner

kennethreitz commented Jun 29, 2012

Thanks!

kennethreitz added a commit that referenced this pull request Jun 29, 2012

Merge pull request #629 from saschpe/develop
Add openSUSE certificate support

@kennethreitz kennethreitz merged commit 065caa3 into requests:develop Jun 29, 2012

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