Merge lp:~bzr/bzr/urllib-verifies-ssl-certs into lp:bzr/2.5

[Vincent Ladeuil]

Verify ssl certs for the http urllib implementation.

THis builds on top of lp:~jelmer/bzr/urllib-verifies-ssl-certs to fix the
tests failures.

There were two kinds of hangs:

- from the client because the ssl errors weren't properly interpreted as
  *not* transient (and were retried against a server which had no idea what
  the client would want ;)

- from the server (a bit surprising since such hangs shouldn't occur anymore
  ?) because if the ssl handshake fails the request shouldn't be processed
  (but the exception needed to be caught to recognize this).

In the future, we probably want to make ssl.ca_certs and ssl.cert_reqs
authentication config options so that proxies can use different options but
that in itself is also quite invasive and out of scope here. These are my
excuses for the semi-hackish way the ca_certs are propagated from the
transport to the _urllib2_wrappers classes. In the end, the plan is to make
them part of auth and proxy_auth credentials which will be a bit less
invasive and open to other options (self signed certificates for example).

Despite its docstring, match_hostname accept our test server 127.0.0.1 IP
address without a wink, I didn't investigate whether it's by luck or by
design. Any ideas ?

Anyway, this seems the fastest way to get certificate verification landed
for 2.5.

[Jelmer Vernooij (jelmer)]

line 69:
+ # FIXME: We proabaly want more tests to capture which ssl
probably?

I also think this is fine for 2.5, so let's JFLI.

review: Approve

[Vincent Ladeuil (vila)]

sent to pqm by email

=== modified file 'bzrlib/plugins/launchpad/lp_registration.py'
--- bzrlib/plugins/launchpad/lp_registration.py	2011-12-19 10:58:39 +0000
+++ bzrlib/plugins/launchpad/lp_registration.py	2012-01-20 13:52:48 +0000
@@ -56,11 +56,7 @@
 class XMLRPCTransport(xmlrpclib.Transport):
 
     def __init__(self, scheme):
-        # In python2.4 xmlrpclib.Transport is a old-style class, and does not
-        # define __init__, so we check first
-        init = getattr(xmlrpclib.Transport, '__init__', None)
-        if init is not None:
-            init(self)
+        xmlrpclib.Transport.__init__(self)
         self._scheme = scheme
         self._opener = _urllib2_wrappers.Opener()
         self.verbose = 0

=== modified file 'bzrlib/plugins/launchpad/test_lp_directory.py'
--- bzrlib/plugins/launchpad/test_lp_directory.py	2011-10-06 06:45:46 +0000
+++ bzrlib/plugins/launchpad/test_lp_directory.py	2012-01-20 13:52:48 +0000
@@ -19,6 +19,7 @@
 import os
 import xmlrpclib
 
+import bzrlib
 from bzrlib import (
     debug,
     errors,
@@ -29,6 +30,7 @@
 from bzrlib.directory_service import directories
 from bzrlib.tests import (
     features,
+    ssl_certs,
     TestCaseInTempDir,
     TestCaseWithMemoryTransport
 )
@@ -452,12 +454,15 @@
         tests.TestCase.setUp(self)
         self.server = self.server_class()
         self.server.start_server()
+        self.addCleanup(self.server.stop_server)
         # Ensure we don't clobber env
         self.overrideEnv('BZR_LP_XMLRPC_URL', None)
-
-    def tearDown(self):
-        self.server.stop_server()
-        tests.TestCase.tearDown(self)
+        # Ensure we use the right certificates for https.
+        # FIXME: There should be a better way but the only alternative I can
+        # think of involves carrying the ca_certs through the lp_registration
+        # infrastructure to _urllib2_wrappers... -- vila 2012-01-20
+        bzrlib.global_state.cmdline_overrides._from_cmdline(
+            ['ssl.ca_certs=%s' % ssl_certs.build_path('ca.crt')])
 
     def set_canned_response(self, server, path):
         response_format = '''HTTP/1.1 200 OK\r

=== modified file 'bzrlib/tests/https_server.py'
--- bzrlib/tests/https_server.py	2011-01-10 22:20:12 +0000
+++ bzrlib/tests/https_server.py	2012-01-20 13:52:48 +0000
@@ -49,7 +49,13 @@
         serving = test_server.TestingTCPServerMixin.verify_request(
             self, request, client_address)
         if serving:
-            request.do_handshake()
+            try:
+                request.do_handshake()
+            except ssl.SSLError, e:
+                # FIXME: We proabaly want more tests to capture which ssl
+                # errors are worth reporting but mostly our tests want an https
+                # server that works -- vila 2012-01-19
+                return False
         return serving
 
     def ignored_exceptions_during_shutdown(self, e):

=== modified file 'bzrlib/tests/test_http.py'
--- bzrlib/tests/test_http.py	2012-01-20 13:52:46 +0000
+++ bzrlib/tests/test_http.py	2012-01-20 13:52:48 +0000
@@ -127,22 +127,29 @@
         ('urllib,http', dict(_activity_server=ActivityHTTPServer,
                             _transport=_urllib.HttpTransport_urllib,)),
         ]
+    if features.pycurl.available():
+        activity_scenarios.append(
+            ('pycurl,http', dict(_activity_server=ActivityHTTPServer,
+                                _transport=PyCurlTransport,)),)
     if features.HTTPSServerFeature.available():
+        # FIXME: Until we have a better way to handle self-signed certificates
+        # (like allowing them in a test specific authentication.conf for
+        # example), we need some specialized pycurl/urllib transport for tests.
+        # -- vila 2012-01-20
+        from bzrlib.tests import (
+            ssl_certs,
+            )
+        class HTTPS_urllib_transport(_urllib.HttpTransport_urllib):
+
+            def __init__(self, base, _from_transport=None):
+                super(HTTPS_urllib_transport, self).__init__(
+                    base, _from_transport=_from_transport,
+                    ca_certs=ssl_certs.build_path('ca.crt'))
+
         activity_scenarios.append(
             ('urllib,https', dict(_activity_server=ActivityHTTPSServer,
-                                _transport=_urllib.HttpTransport_urllib,)),)
-    if features.pycurl.available():
-        activity_scenarios.append(
-            ('pycurl,http', dict(_activity_server=ActivityHTTPServer,
-                                _transport=PyCurlTransport,)),)
-        if features.HTTPSServerFeature.available():
-            from bzrlib.tests import (
-                ssl_certs,
-                )
-            # FIXME: Until we have a better way to handle self-signed
-            # certificates (like allowing them in a test specific
-            # authentication.conf for example), we need some specialized pycurl
-            # transport for tests.
+                                  _transport=HTTPS_urllib_transport,)),)
+        if features.pycurl.available():
             class HTTPS_pycurl_transport(PyCurlTransport):
 
                 def __init__(self, base, _from_transport=None):
@@ -2119,18 +2126,17 @@
         tests.TestCase.setUp(self)
         self.server = self._activity_server(self._protocol_version)
         self.server.start_server()
+        self.addCleanup(self.server.stop_server)
         _activities = {} # Don't close over self and create a cycle
         def report_activity(t, bytes, direction):
             count = _activities.get(direction, 0)
             count += bytes
             _activities[direction] = count
         self.activities = _activities
-
         # We override at class level because constructors may propagate the
         # bound method and render instance overriding ineffective (an
         # alternative would be to define a specific ui factory instead...)
         self.overrideAttr(self._transport, '_report_activity', report_activity)
-        self.addCleanup(self.server.stop_server)
 
     def get_transport(self):
         t = self._transport(self.server.get_url())

=== modified file 'bzrlib/tests/test_https_urllib.py'
--- bzrlib/tests/test_https_urllib.py	2012-01-20 13:52:46 +0000
+++ bzrlib/tests/test_https_urllib.py	2012-01-20 13:52:48 +0000
@@ -18,18 +18,17 @@
 
 """
 
+import os
 import ssl
 
-from bzrlib import trace
+from bzrlib import (
+    config,
+    trace,
+    )
 from bzrlib.errors import (
     CertificateError,
     ConfigOptionValueError,
     )
-from bzrlib.config import (
-    IniFileStore,
-    Stack,
-    )
-import os
 from bzrlib.tests import (
     TestCase,
     TestCaseInTempDir,
@@ -37,23 +36,20 @@
 from bzrlib.transport.http import _urllib2_wrappers
 
 
-def stack_from_string(text):
-    store = IniFileStore()
-    store._load_from_string(text)
-    return Stack([store.get_sections])
-
-
 class CaCertsConfigTests(TestCaseInTempDir):
 
+    def get_stack(self, content):
+        return config.MemoryStack(content.encode('utf-8'))
+
     def test_default_raises_value_error(self):
-        stack = stack_from_string("")
+        stack = self.get_stack("")
         self.overrideAttr(_urllib2_wrappers, "DEFAULT_CA_PATH",
                 "/i-do-not-exist")
         self.assertRaises(ValueError, stack.get, 'ssl.ca_certs')
 
     def test_default_exists(self):
         self.build_tree(['cacerts.pem'])
-        stack = stack_from_string("")
+        stack = self.get_stack("")
         path = os.path.join(self.test_dir, "cacerts.pem")
         self.overrideAttr(_urllib2_wrappers, "DEFAULT_CA_PATH", path)
         self.assertEquals(path, stack.get('ssl.ca_certs'))
@@ -61,49 +57,53 @@
     def test_specified(self):
         self.build_tree(['cacerts.pem'])
         path = os.path.join(self.test_dir, "cacerts.pem")
-        stack = stack_from_string("ssl.ca_certs = %s\n" % path)
+        stack = self.get_stack("ssl.ca_certs = %s\n" % path)
         self.assertEquals(path, stack.get('ssl.ca_certs'))
 
     def test_specified_doesnt_exist(self):
         path = os.path.join(self.test_dir, "nonexisting.pem")
-        stack = stack_from_string("ssl.ca_certs = %s\n" % path)
+        stack = self.get_stack("ssl.ca_certs = %s\n" % path)
         self.warnings = []
         def warning(*args):
             self.warnings.append(args[0] % args[1:])
         self.overrideAttr(trace, 'warning', warning)
-        self.assertEquals(_urllib2_wrappers.DEFAULT_CA_PATH, stack.get('ssl.ca_certs'))
+        self.assertEquals(_urllib2_wrappers.DEFAULT_CA_PATH,
+                          stack.get('ssl.ca_certs'))
         self.assertLength(1, self.warnings)
-        self.assertContainsRe(self.warnings[0], "is not valid for \"ssl.ca_certs\"")
+        self.assertContainsRe(self.warnings[0],
+                              "is not valid for \"ssl.ca_certs\"")
 
 
 class CertReqsConfigTests(TestCaseInTempDir):
 
     def test_default(self):
-        stack = stack_from_string("")
+        stack = config.MemoryStack("")
         self.assertEquals(ssl.CERT_REQUIRED, stack.get("ssl.cert_reqs"))
 
     def test_from_string(self):
-        stack = stack_from_string("ssl.cert_reqs = none\n")
+        stack = config.MemoryStack("ssl.cert_reqs = none\n")
         self.assertEquals(ssl.CERT_NONE, stack.get("ssl.cert_reqs"))
-        stack = stack_from_string("ssl.cert_reqs = optional\n")
+        stack = config.MemoryStack("ssl.cert_reqs = optional\n")
         self.assertEquals(ssl.CERT_OPTIONAL, stack.get("ssl.cert_reqs"))
-        stack = stack_from_string("ssl.cert_reqs = required\n")
+        stack = config.MemoryStack("ssl.cert_reqs = required\n")
         self.assertEquals(ssl.CERT_REQUIRED, stack.get("ssl.cert_reqs"))
-        stack = stack_from_string("ssl.cert_reqs = invalid\n")
+        stack = config.MemoryStack("ssl.cert_reqs = invalid\n")
         self.assertRaises(ConfigOptionValueError, stack.get, "ssl.cert_reqs")
 
 
 class MatchHostnameTests(TestCase):
 
     def test_no_certificate(self):
-        self.assertRaises(ValueError, _urllib2_wrappers.match_hostname({}))
+        self.assertRaises(ValueError,
+                          _urllib2_wrappers.match_hostname, {}, "example.com")
 
     def test_no_valid_attributes(self):
-        self.assertRaises(CertificateError,
-            _urllib2_wrappers.match_hostname({"Problem": "Solved"}))
+        self.assertRaises(CertificateError, _urllib2_wrappers.match_hostname,
+                          {"Problem": "Solved"}, "example.com")
 
     def test_common_name(self):
         cert = {'subject': ((('commonName', 'example.com'),),)}
-        self.assertIs(None, _urllib2_wrappers.match_hostname(cert, "example.com"))
+        self.assertIs(None,
+                      _urllib2_wrappers.match_hostname(cert, "example.com"))
         self.assertRaises(CertificateError, _urllib2_wrappers.match_hostname,
-                cert, "example.org")
+                          cert, "example.org")

=== modified file 'bzrlib/transport/__init__.py'
--- bzrlib/transport/__init__.py	2012-01-20 13:52:46 +0000
+++ bzrlib/transport/__init__.py	2012-01-20 13:52:48 +0000
@@ -1782,15 +1782,15 @@
                  help="Read-only access of branches exported on the web.")
 register_transport_proto('https://',
             help="Read-only access of branches exported on the web using SSL.")
-# The default http implementation is urllib
+# The default http implementation is urllib, but https uses pycurl if available
 register_lazy_transport('http://', 'bzrlib.transport.http._pycurl',
                         'PyCurlTransport')
-register_lazy_transport('https://', 'bzrlib.transport.http._pycurl',
-                        'PyCurlTransport')
 register_lazy_transport('http://', 'bzrlib.transport.http._urllib',
                         'HttpTransport_urllib')
 register_lazy_transport('https://', 'bzrlib.transport.http._urllib',
                         'HttpTransport_urllib')
+register_lazy_transport('https://', 'bzrlib.transport.http._pycurl',
+                        'PyCurlTransport')
 
 register_transport_proto('ftp://', help="Access using passive FTP.")
 register_lazy_transport('ftp://', 'bzrlib.transport.ftp', 'FtpTransport')

=== modified file 'bzrlib/transport/http/_urllib.py'
--- bzrlib/transport/http/_urllib.py	2011-12-18 15:28:38 +0000
+++ bzrlib/transport/http/_urllib.py	2012-01-20 13:52:48 +0000
@@ -38,14 +38,14 @@
 
     _opener_class = Opener
 
-    def __init__(self, base, _from_transport=None):
+    def __init__(self, base, _from_transport=None, ca_certs=None):
         super(HttpTransport_urllib, self).__init__(
             base, 'urllib', _from_transport=_from_transport)
         if _from_transport is not None:
             self._opener = _from_transport._opener
         else:
             self._opener = self._opener_class(
-                report_activity=self._report_activity)
+                report_activity=self._report_activity, ca_certs=ca_certs)
 
     def _perform(self, request):
         """Send the request to the server and handles common errors.
@@ -175,7 +175,18 @@
         )
     permutations = [(HttpTransport_urllib, http_server.HttpServer_urllib),]
     if features.HTTPSServerFeature.available():
-        from bzrlib.tests import https_server
-        permutations.append((HttpTransport_urllib,
+        from bzrlib.tests import (
+            https_server,
+            ssl_certs,
+            )
+
+        class HTTPS_urllib_transport(HttpTransport_urllib):
+
+            def __init__(self, base, _from_transport=None):
+                super(HTTPS_urllib_transport, self).__init__(
+                    base, _from_transport=_from_transport,
+                    ca_certs=ssl_certs.build_path('ca.crt'))
+
+        permutations.append((HTTPS_urllib_transport,
                              https_server.HTTPSServer_urllib))
     return permutations

=== modified file 'bzrlib/transport/http/_urllib2_wrappers.py'
--- bzrlib/transport/http/_urllib2_wrappers.py	2012-01-20 13:52:46 +0000
+++ bzrlib/transport/http/_urllib2_wrappers.py	2012-01-20 13:52:48 +0000
@@ -122,7 +122,7 @@
 Whether to require a certificate from the remote side. (default:required)
 
 Possible values:
- * none: certificates ignored
+ * none: Certificates ignored
  * optional: Certificates not required, but validated if provided
  * required: Certificates required, and validated
 """)
@@ -356,11 +356,12 @@
 
     # XXX: Needs refactoring at the caller level.
     def __init__(self, host, port=None, proxied_host=None,
-                 report_activity=None):
+                 report_activity=None, ca_certs=None):
         AbstractHTTPConnection.__init__(self, report_activity=report_activity)
         # Use strict=True since we don't support HTTP/0.9
         httplib.HTTPConnection.__init__(self, host, port, strict=True)
         self.proxied_host = proxied_host
+        # ca_certs is ignored, it's only relevant for https
 
     def connect(self):
         if 'http' in debug.debug_flags:
@@ -413,13 +414,12 @@
                         return
                     dnsnames.append(value)
     if len(dnsnames) > 1:
-        raise errors.CertificateError("hostname %r "
-            "doesn't match either of %s"
+        raise errors.CertificateError(
+            "hostname %r doesn't match either of %s"
             % (hostname, ', '.join(map(repr, dnsnames))))
     elif len(dnsnames) == 1:
-        raise errors.CertificateError("hostname %r "
-            "doesn't match %r"
-            % (hostname, dnsnames[0]))
+        raise errors.CertificateError("hostname %r doesn't match %r" %
+                                      (hostname, dnsnames[0]))
     else:
         raise errors.CertificateError("no appropriate commonName or "
             "subjectAltName fields were found")
@@ -429,12 +429,13 @@
 
     def __init__(self, host, port=None, key_file=None, cert_file=None,
                  proxied_host=None,
-                 report_activity=None):
+                 report_activity=None, ca_certs=None):
         AbstractHTTPConnection.__init__(self, report_activity=report_activity)
         # Use strict=True since we don't support HTTP/0.9
         httplib.HTTPSConnection.__init__(self, host, port,
                                          key_file, cert_file, strict=True)
         self.proxied_host = proxied_host
+        self.ca_certs = ca_certs
 
     def connect(self):
         if 'http' in debug.debug_flags:
@@ -447,7 +448,10 @@
     def connect_to_origin(self):
         # FIXME JRV 2011-12-18: Use location config here?
         config_stack = config.GlobalStack()
-        ca_certs = config_stack.get('ssl.ca_certs')
+        if self.ca_certs is None:
+            ca_certs = config_stack.get('ssl.ca_certs')
+        else:
+            ca_certs = self.ca_certs
         cert_reqs = config_stack.get('ssl.cert_reqs')
         if cert_reqs == ssl.CERT_NONE:
             trace.warning("not checking SSL certificates for %s: %d",
@@ -581,8 +585,9 @@
 
     handler_order = 1000 # after all pre-processings
 
-    def __init__(self, report_activity=None):
+    def __init__(self, report_activity=None, ca_certs=None):
         self._report_activity = report_activity
+        self.ca_certs = ca_certs
 
     def create_connection(self, request, http_connection_class):
         host = request.get_host()
@@ -596,7 +601,8 @@
         try:
             connection = http_connection_class(
                 host, proxied_host=request.proxied_host,
-                report_activity=self._report_activity)
+                report_activity=self._report_activity,
+                ca_certs=self.ca_certs)
         except httplib.InvalidURL, exception:
             # There is only one occurrence of InvalidURL in httplib
             raise errors.InvalidURL(request.get_full_url(),
@@ -788,6 +794,10 @@
                     % (request, request.connection.sock.getsockname())
             response = connection.getresponse()
             convert_to_addinfourl = True
+        except (ssl.SSLError, errors.CertificateError):
+            # Something is wrong with either the certificate or the hostname,
+            # re-trying won't help
+            raise
         except (socket.gaierror, httplib.BadStatusLine, httplib.UnknownProtocol,
                 socket.error, httplib.HTTPException):
             response = self.retry_or_raise(http_class, request, first_try)
@@ -1785,9 +1795,10 @@
                  connection=ConnectionHandler,
                  redirect=HTTPRedirectHandler,
                  error=HTTPErrorProcessor,
-                 report_activity=None):
+                 report_activity=None,
+                 ca_certs=None):
         self._opener = urllib2.build_opener(
-            connection(report_activity=report_activity),
+            connection(report_activity=report_activity, ca_certs=ca_certs),
             redirect, error,
             ProxyHandler(),
             HTTPBasicAuthHandler(),

=== modified file 'doc/en/release-notes/bzr-2.5.txt'
--- doc/en/release-notes/bzr-2.5.txt	2012-01-20 13:52:46 +0000
+++ doc/en/release-notes/bzr-2.5.txt	2012-01-20 13:52:48 +0000
@@ -53,6 +53,10 @@
 * Test for equality instead of object identity where ROOT_PARENT is concerned.
   (Wouter van Heyst, #881142)
 
+* urllib-based HTTPS client connections now verify the server certificate
+  validity as well as the hostname.
+  (Jelmer Vernooij, Vincent Ladeuil, #651161)
+
 Documentation
 *************
 
@@ -540,12 +544,6 @@
   to 'line-based'. This replace the previous shelf UI only patch using
   ``INSIDE_EMACS``. (Benoît Pierre)
 
-* urllib-based HTTPS client connections now verify the server certificate
-  as well as the hostname. (Jelmer Vernooij)
-
-* urllib-based HTTPS connections are now the default.
-  (Jelmer Vernooij, #125055, #651161)
-
 Bug Fixes
 *********