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

[Vincent Ladeuil]

Provide an ``ssl.ca_certs`` default value for supported platforms.

Resubmitting a slightly simpler implementation to ensure we're still agreeing, see my replies to previous reviews too.

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

[Vincent Ladeuil (vila)]

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

Right, trying to fix that led to too much complications which I want to
avoid for 2.5. So, as discussed on IRC, I've file http:/pad.lv/924220 to
support 'optional' later and just removed it from the actual implementation.

... and I'll take care of the news conflicts when/if this proposal is approved ;)

[Martin Packman (gz)]

Trying a value relative to sys.executable is right for the installer, but what is absolutely required is the pretty message in the case where there are no certs, rather than what I currently get with this branch:

C:\bzr\bzr\920455-ssl-defaults>%PY27% bzr info https://bazaar.launchpad.net/
bzr: ERROR: Bad value "C:\Dev\python\2.7\PCbuild\ca_bundle.crt" for option "ssl.ca_certs".

That's no good to man or beast, if it can't validate the cert, it *must* say what needs fixing, and how to disable the validation. This nearly exists already, but the logic seems to have been broken so the "no valid trusted SSL CA certificates file set" message branch doesn't get hit:

<https://code.launchpad.net/~jelmer/bzr/urllib-verifies-ssl-certs/+merge/86360>

What I expect is something like:

    >bzr info https://bazaar.launchpad.net/
    bzr: ERROR: No valid trusted SSL CA certificates file found.

    See `bzr help ssl.ca_certs` for how to specify trusted CA certificates.
    Pass -Ossl.cert_reqs=none to disable certificate verification entirely.

We nearly have all the bits, they just need connecting and tidying up.

review: Needs Fixing

[Martin Packman (gz)]

With changes, the output is now:

C:\bzr\bzr\920455-ssl-defaults>%PY27% bzr info https://bazaar.launchpad.net/
Value "C:\Dev\python\2.7\PCbuild\ca_bundle.crt" is not valid for "ssl.ca_certs"
No valid trusted SSL CA certificates file set. See 'bzr help ssl.ca_certs' for more information on setting trusted CAs.
bzr: ERROR: _ssl.c:331: No root certificates specified for verification of other-side certificates.

Still not totally ideal but is good enough and I an poke later when some other exception stuff is sorted.

review: Approve

[Vincent Ladeuil (vila)]

sent to pqm by email

=== modified file 'bzrlib/errors.py'
--- bzrlib/errors.py	2012-01-27 15:47:12 +0000
+++ bzrlib/errors.py	2012-01-31 17:04:31 +0000
@@ -1761,7 +1761,8 @@
 
 class ConfigOptionValueError(BzrError):
 
-    _fmt = """Bad value "%(value)s" for option "%(name)s"."""
+    _fmt = ('Bad value "%(value)s" for option "%(name)s".\n'
+            'See ``bzr help %(name)s``')
 
     def __init__(self, name, value):
         BzrError.__init__(self, name=name, value=value)
 
=== 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-31 17:04:31 +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'])
@@ -61,14 +53,15 @@
         self.assertEquals(path, stack.get('ssl.ca_certs'))
 
     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)
+        stack = self.get_stack('')
+        # Disable the default value mechanism to force the behavior we want
+        self.overrideAttr(_urllib2_wrappers.opt_ssl_ca_certs, 'default',
+                          os.path.join(self.test_dir, u"nonexisting.pem"))
         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(None, stack.get('ssl.ca_certs'))
         self.assertLength(1, self.warnings)
         self.assertContainsRe(self.warnings[0],
                               "is not valid for \"ssl.ca_certs\"")
@@ -83,8 +76,6 @@
     def test_from_string(self):
         stack = config.MemoryStack("ssl.cert_reqs = none\n")
         self.assertEquals(ssl.CERT_NONE, stack.get("ssl.cert_reqs"))
-        stack = config.MemoryStack("ssl.cert_reqs = optional\n")
-        self.assertEquals(ssl.CERT_OPTIONAL, stack.get("ssl.cert_reqs"))
         stack = config.MemoryStack("ssl.cert_reqs = required\n")
         self.assertEquals(ssl.CERT_REQUIRED, stack.get("ssl.cert_reqs"))
         stack = config.MemoryStack("ssl.cert_reqs = invalid\n")
 
=== 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-31 17:04:31 +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,36 @@
 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':
+        return os.path.join(os.path.dirname(sys.executable), u"ca_bundle.crt")
+    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,16 +112,11 @@
     return path
 
 
-def default_cert_reqs():
-    return u"required"
-
-
 def cert_reqs_from_store(unicode_str):
     import ssl
     try:
         return {
             "required": ssl.CERT_REQUIRED,
-            "optional": ssl.CERT_OPTIONAL,
             "none": ssl.CERT_NONE
             }[unicode_str]
     except KeyError:
@@ -112,10 +129,15 @@
         invalid='warning',
         help="""\
 Path to certification authority certificates to trust.
+
+This should be a valid path to a bundle containing all root Certificate
+Authorities used to verify an https server certificate.
+
+Use ssl.cert_reqs=none to disable certificate verification.
 """)
 
 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="""\
@@ -123,8 +145,7 @@
 
 Possible values:
  * none: Certificates ignored
- * optional: Certificates not required, but validated if provided
- * required: Certificates required, and validated
+ * required: Certificates required and validated
 """)
 
 checked_kerberos = False
@@ -448,34 +469,34 @@
     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",
+            trace.warning("Not checking SSL certificate 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 "
+                    "No valid trusted SSL CA certificates file set. See "
                     "'bzr help ssl.ca_certs' for more information on setting "
-                    "trusted CA's.")
+                    "trusted CAs.")
         try:
             ssl_sock = ssl.wrap_socket(self.sock, self.key_file, self.cert_file,
                 cert_reqs=cert_reqs, ca_certs=ca_certs)
         except ssl.SSLError, e:
-            if e.errno != ssl.SSL_ERROR_SSL:
-                raise
             trace.note(
-                "To disable SSL certificate verification, use "
-                "-Ossl.cert_reqs=none. See ``bzr help ssl.ca_certs`` for "
-                "more information on specifying trusted CA certificates.")
+                "\n"
+                "See `bzr help ssl.ca_certs` for how to specify trusted CA"
+                "certificates.\n"
+                "Pass -Ossl.cert_reqs=none to disable certificate "
+                "verification entirely.\n")
             raise
-        peer_cert = ssl_sock.getpeercert()
-        if (cert_reqs == ssl.CERT_REQUIRED or
-            (cert_reqs == ssl.CERT_OPTIONAL and peer_cert)):
+        if cert_reqs == ssl.CERT_REQUIRED:
+            peer_cert = ssl_sock.getpeercert()
             match_hostname(peer_cert, self.host)
 
         # Wrap the ssl socket before anybody use it
 
=== modified file 'doc/en/release-notes/bzr-2.5.txt'
--- doc/en/release-notes/bzr-2.5.txt	2012-01-31 15:43:17 +0000
+++ doc/en/release-notes/bzr-2.5.txt	2012-01-31 17:04:31 +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)