Merge lp:~vila/bzr/920455-ssl-defaults into lp:bzr/2.5

[Vincent Ladeuil]

Provide platform specific default values for ssl.ca_certs

This add default values for the ssl.ca_certs config option pointing to the
most probable place where the certificates are place for supported
platforms.

Feedback needed from windows and osx packagers unless we rely on them to fix
it when building 2.5.0...

I've changed the tests so at least test_default_exists
bzrlib/tests/test_https_urllib.py fails if the default value is wrong.

I've also change the option to fail if a non-existing path is used and
changed the code to check the config option only if ssl.cert_reqs is not none.

With these changes, either:

- the certificates are there and they will be checked by default

- they are not but ssl.cert_reqs is none, no need to bother the user
  especially since that is the obvious workaround for now if something goes
  wrong with the verification,

- an error is raised if the user ask for verification but we can't find the
  CAs.

[Vincent Ladeuil (vila)]

@gz, Gordon: Feedback on where you expect to install the bundled ca certs expected ;)

[Martin Packman (gz)]

So, on windows we're mostly stuffed because ideally we'd use the Internet Explorer certificate store, but that's a completely different interface to openssl and the cert dir used on nix systems.

The best we can do is bundle curl-ca-bundle.crt with the all-in-one installer like we did previously:

<http://wiki.bazaar.canonical.com/BzrWin32Installer#bzr-dependencies>

Then accept the fact branching over https will be broken for everyone else using a python installer or running setup.py themselves.

This is why I was picky about the error message given for the original change... unfortunately it's now just a ValueError and traceback due to the default config value being bad. This branch changes one of those but not the other.

[Jelmer Vernooij (jelmer)]

32 def test_specified_doesnt_exist(self):
33 path = os.path.join(self.test_dir, "nonexisting.pem")
34 stack = self.get_stack("ssl.ca_certs = %s\n" % path)
35 - self.warnings = []
36 - def warning(*args):
37 - self.warnings.append(args[0] % args[1:])
38 - self.overrideAttr(trace, 'warning', warning)
39 - self.assertEquals(_urllib2_wrappers.DEFAULT_CA_PATH,
40 - stack.get('ssl.ca_certs'))
41 - self.assertLength(1, self.warnings)
42 - self.assertContainsRe(self.warnings[0],
43 - "is not valid for \"ssl.ca_certs\"")
44 + self.assertRaises(ConfigOptionValueError, stack.get, 'ssl.ca_certs')
Should it really be an error if the ssl.ca_certs path doesn't exist? What if e.g. "ssl.ca_reqs = optional", it doesn't seem like it should be a problem if the ca_certs are missing.

default_ca_cert() seems to return "/etc/ssl/..." on Windows. This seems wrong in any case. I realize you've asked for feedback from the packagers, but I think we should raise ValueError or return None for now at least.

review: Approve

[Vincent Ladeuil (vila)]

> This is why I was picky about the error message given for the original
> change... unfortunately it's now just a ValueError and traceback due to the
> default config value being bad. This branch changes one of those but not the
> other.

No, please do try again (with this proposal):

  bzr config --remove launchpad_username
  bzr config ssl.ca_certs=/I-dont-exist

vila:~/src/bzr/bugs/920455-ssl-defaults :) $ ./bzr info lp:bzr
bzr: ERROR: Bad value "/I-dont-exist" for option "ssl.ca_certs".

So you get a proper error message that we may want to clarify but you don't
get a traceback.

You got a traceback prior to this change because default_ca_certs() was
raising it which is why I changed the implementation as ValueError is caught
for the *_from_store() functions, not the _default() functions.

I could change the ConfigValueError message to include 'See bzr help
ssl.ca_certs' but we froze the strings for 2.5 so I'd rather do that for
trunk.

I'll file bugs for windows and osx installers to make sure a bundle is
included.

[Vincent Ladeuil (vila)]

    > Review: Approve
    > 32 def test_specified_doesnt_exist(self):
    > 33 path = os.path.join(self.test_dir, "nonexisting.pem")
    > 34 stack = self.get_stack("ssl.ca_certs = %s\n" % path)
    > 35 - self.warnings = []
    > 36 - def warning(*args):
    > 37 - self.warnings.append(args[0] % args[1:])
    > 38 - self.overrideAttr(trace, 'warning', warning)
    > 39 - self.assertEquals(_urllib2_wrappers.DEFAULT_CA_PATH,
    > 40 - stack.get('ssl.ca_certs'))
    > 41 - self.assertLength(1, self.warnings)
    > 42 - self.assertContainsRe(self.warnings[0],
    > 43 - "is not valid for \"ssl.ca_certs\"")
    > 44 + self.assertRaises(ConfigOptionValueError, stack.get, 'ssl.ca_certs')

    > Should it really be an error if the ssl.ca_certs path doesn't
    > exist? What if e.g. "ssl.ca_reqs = optional", it doesn't seem like
    > it should be a problem if the ca_certs are missing.

Indeed ! That's why I also change the code to query the option only if
required ;)

    > default_ca_cert() seems to return "/etc/ssl/..." on Windows. This
    > seems wrong in any case.

Yes.

    > I realize you've asked for feedback from the packagers, but I
    > think we should raise ValueError or return None for now at least.

That's what will happen if the option is needed, see
ca_certs_from_store() and invalid='error' ! ;)

[Jelmer Vernooij (jelmer)]

On 01/30/2012 03:19 PM, Vincent Ladeuil wrote:
> > Review: Approve
> > 32 def test_specified_doesnt_exist(self):
> > 33 path = os.path.join(self.test_dir, "nonexisting.pem")
> > 34 stack = self.get_stack("ssl.ca_certs = %s\n" % path)
> > 35 - self.warnings = []
> > 36 - def warning(*args):
> > 37 - self.warnings.append(args[0] % args[1:])
> > 38 - self.overrideAttr(trace, 'warning', warning)
> > 39 - self.assertEquals(_urllib2_wrappers.DEFAULT_CA_PATH,
> > 40 - stack.get('ssl.ca_certs'))
> > 41 - self.assertLength(1, self.warnings)
> > 42 - self.assertContainsRe(self.warnings[0],
> > 43 - "is not valid for \"ssl.ca_certs\"")
> > 44 + self.assertRaises(ConfigOptionValueError, stack.get, 'ssl.ca_certs')
>
> > Should it really be an error if the ssl.ca_certs path doesn't
> > exist? What if e.g. "ssl.ca_reqs = optional", it doesn't seem like
> > it should be a problem if the ca_certs are missing.
>
> Indeed ! That's why I also change the code to query the option only if
> required ;)
You're (correctly) trying to retrieve the ca certs too if
ssl.ca_reqs=optional, but if the ca certs path doesn't exist, that
becomes a terminal error. It shouldn't be if ssl.ca_reqs=optional, only
if ssl.ca_reqs=required.
>
>
> > I realize you've asked for feedback from the packagers, but I
> > think we should raise ValueError or return None for now at least.
>
> That's what will happen if the option is needed, see
> ca_certs_from_store() and invalid='error' ! ;)
I don't see how that's the case. We'll be trying to retrieve the ca
certs in that case and it'll cause ConfigOptionValueError to be raised
(and bzr to be aborted), right?

Cheers,

Jelmer

=== modified file 'bzrlib/tests/test_https_urllib.py'
--- bzrlib/tests/test_https_urllib.py	2012-01-19 15:27:47 +0000
+++ bzrlib/tests/test_https_urllib.py	2012-01-30 14:24:24 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2011 Canonical Ltd
+# Copyright (C) 2011,2012 Canonical Ltd
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -41,18 +41,10 @@
     def get_stack(self, content):
         return config.MemoryStack(content.encode('utf-8'))
 
-    def test_default_raises_value_error(self):
-        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'])
+        """Check that the default we provide exists for the tested platform."""
         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'))
+        self.assertPathExists(stack.get('ssl.ca_certs'))
 
     def test_specified(self):
         self.build_tree(['cacerts.pem'])
@@ -63,15 +55,7 @@
     def test_specified_doesnt_exist(self):
         path = os.path.join(self.test_dir, "nonexisting.pem")
         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.assertLength(1, self.warnings)
-        self.assertContainsRe(self.warnings[0],
-                              "is not valid for \"ssl.ca_certs\"")
+        self.assertRaises(ConfigOptionValueError, stack.get, 'ssl.ca_certs')
 
 
 class CertReqsConfigTests(TestCaseInTempDir):

=== modified file 'bzrlib/transport/http/_urllib2_wrappers.py'
--- bzrlib/transport/http/_urllib2_wrappers.py	2012-01-20 09:19:14 +0000
+++ bzrlib/transport/http/_urllib2_wrappers.py	2012-01-30 14:24:24 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2006-2011 Canonical Ltd
+# Copyright (C) 2006-2012 Canonical Ltd
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -74,14 +74,39 @@
 import ssl
 """)
 
-DEFAULT_CA_PATH = u"/etc/ssl/certs/ca-certificates.crt"
 
+# Note for packagers: if there is no package providing certs for your platform,
+# the curl project produces http://curl.haxx.se/ca/cacert.pem weekly.
+ssl_ca_certs_known_locations = [
+    u'/etc/ssl/certs/ca-certificates.crt', # Ubuntu/debian/gentoo
+    u'/etc/pki/tls/certs/ca-bundle.crt', # Fedora/CentOS/RH
+    u'/etc/ssl/ca-bundle.pem', # OpenSuse
+    u'/etc/ssl/cert.pem', # OpenSuse
+    u"/usr/local/share/certs/ca-root-nss.crt", # FreeBSD
+    # XXX: Needs checking, can't trust the interweb ;) -- vila 2012-01-25
+    u'/etc/openssl/certs/ca-certificates.crt', # Solaris
+    ]
 
 def default_ca_certs():
-    if not os.path.exists(DEFAULT_CA_PATH):
-        raise ValueError("default ca certs path %s does not exist" %
-            DEFAULT_CA_PATH)
-    return DEFAULT_CA_PATH
+    if sys.platform == 'win32':
+        # FIXME: We could reuse bzrlib.transport.http.ca_bundle but import that
+        # here sounds... too hackish. Waiting for the windows installer guys
+        # feedback on which path to use -- vila 2012-01-25
+        pass
+    elif sys.platform == 'darwin':
+        # FIXME: Needs some default value for osx, waiting for osx installers
+        # guys feedback -- vila 2012-01-25
+        pass
+    else:
+        # Try known locations for friendly OSes providing the root certificates
+        # without making them hard to use for any https client.
+        for path in ssl_ca_certs_known_locations:
+            if os.path.exists(path):
+                # First found wins
+                return path
+    # A default path that makes sense and will be mentioned in the error
+    # presented to the user, even if not correct for all platforms
+    return ssl_ca_certs_known_locations[0]
 
 
 def ca_certs_from_store(path):
@@ -90,10 +115,6 @@
     return path
 
 
-def default_cert_reqs():
-    return u"required"
-
-
 def cert_reqs_from_store(unicode_str):
     import ssl
     try:
@@ -109,13 +130,13 @@
 opt_ssl_ca_certs = config.Option('ssl.ca_certs',
         from_unicode=ca_certs_from_store,
         default=default_ca_certs,
-        invalid='warning',
+        invalid='error',
         help="""\
 Path to certification authority certificates to trust.
 """)
 
 opt_ssl_cert_reqs = config.Option('ssl.cert_reqs',
-        default=default_cert_reqs,
+        default=u"required",
         from_unicode=cert_reqs_from_store,
         invalid='error',
         help="""\
@@ -448,15 +469,16 @@
     def connect_to_origin(self):
         # FIXME JRV 2011-12-18: Use location config here?
         config_stack = config.GlobalStack()
-        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",
                 self.host, self.port)
+            ca_certs = None
         else:
+            if self.ca_certs is None:
+                ca_certs = config_stack.get('ssl.ca_certs')
+            else:
+                ca_certs = self.ca_certs
             if ca_certs is None:
                 trace.warning(
                     "no valid trusted SSL CA certificates file set. See "

=== modified file 'doc/en/release-notes/bzr-2.5.txt'
--- doc/en/release-notes/bzr-2.5.txt	2012-01-28 00:41:30 +0000
+++ doc/en/release-notes/bzr-2.5.txt	2012-01-30 14:24:24 +0000
@@ -56,12 +56,19 @@
 * ``bzr branch`` now fetches revisions when branching into an empty
   control directory. (Jelmer Vernooij, #905594)
 
+<<<<<<< TREE
 * ``bzr branch`` generates correct target branch locations again if not
   specified. (Jelmer Vernooij, #919218)
 
 * ``bzr send`` works on treeless branches again.
   (Jelmer Vernooij, #921591)
 
+=======
+* A sane default is provided for ``ssl.ca_certs`` which should points to the
+  Certificate Authority bundle for supported platforms.
+  (Vincent Ladeuil, #920455)
+
+>>>>>>> MERGE-SOURCE
 * Support scripts that don't call bzrlib.initialize() but still call run_bzr().
   (Vincent Ladeuil, #917733)