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

Proposed by Vincent Ladeuil
Status: Merged
Approved by: Martin Packman
Approved revision: 6463
Merged at revision: 6468
Proposed branch: lp:~vila/bzr/920455-ssl-defaults
Merge into: lp:bzr/2.5
Diff against target: 242 lines (+67/-47) (has conflicts)
4 files modified
bzrlib/errors.py (+2/-1)
bzrlib/tests/test_https_urllib.py (+8/-17)
bzrlib/transport/http/_urllib2_wrappers.py (+50/-29)
doc/en/release-notes/bzr-2.5.txt (+7/-0)
Text conflict in doc/en/release-notes/bzr-2.5.txt
To merge this branch: bzr merge lp:~vila/bzr/920455-ssl-defaults
Reviewer Review Type Date Requested Status
Martin Packman (community) Approve
Gordon Tyler Pending
Jelmer Vernooij Pending
Review via email: mp+90693@code.launchpad.net

This proposal supersedes a proposal from 2012-01-25.

Commit message

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

Description of the change

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.

To post a comment you must log in.
Revision history for this message
Vincent Ladeuil (vila) wrote : Posted in a previous version of this proposal

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

Revision history for this message
Martin Packman (gz) wrote : Posted in a previous version of this proposal

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.

Revision history for this message
Jelmer Vernooij (jelmer) wrote : Posted in a previous version of this proposal

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
Revision history for this message
Vincent Ladeuil (vila) wrote : Posted in a previous version of this proposal

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

Revision history for this message
Vincent Ladeuil (vila) wrote : Posted in a previous version of this proposal

    > 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' ! ;)

Revision history for this message
Jelmer Vernooij (jelmer) wrote : Posted in a previous version of this proposal

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

lp:~vila/bzr/920455-ssl-defaults updated
6461. By Vincent Ladeuil

Get rid of 'optional' for ssl.ca_reqs to simplify the model.

Revision history for this message
Vincent Ladeuil (vila) wrote :

> 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 ;)

lp:~vila/bzr/920455-ssl-defaults updated
6462. By Vincent Ladeuil

mgz said this should be good for windows all-in-one installer.

Revision history for this message
Martin Packman (gz) wrote :

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
lp:~vila/bzr/920455-ssl-defaults updated
6463. By Vincent Ladeuil

Feedback from mgz.

Revision history for this message
Martin Packman (gz) wrote :

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
lp:~vila/bzr/920455-ssl-defaults updated
6464. By Vincent Ladeuil

We need a clearer understanding of ssl errors.

Revision history for this message
Vincent Ladeuil (vila) wrote :

sent to pqm by email

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'bzrlib/errors.py'
2--- bzrlib/errors.py 2012-01-27 15:47:12 +0000
3+++ bzrlib/errors.py 2012-01-31 17:04:31 +0000
4@@ -1761,7 +1761,8 @@
5
6 class ConfigOptionValueError(BzrError):
7
8- _fmt = """Bad value "%(value)s" for option "%(name)s"."""
9+ _fmt = ('Bad value "%(value)s" for option "%(name)s".\n'
10+ 'See ``bzr help %(name)s``')
11
12 def __init__(self, name, value):
13 BzrError.__init__(self, name=name, value=value)
14
15=== modified file 'bzrlib/tests/test_https_urllib.py'
16--- bzrlib/tests/test_https_urllib.py 2012-01-19 15:27:47 +0000
17+++ bzrlib/tests/test_https_urllib.py 2012-01-31 17:04:31 +0000
18@@ -1,4 +1,4 @@
19-# Copyright (C) 2011 Canonical Ltd
20+# Copyright (C) 2011,2012 Canonical Ltd
21 #
22 # This program is free software; you can redistribute it and/or modify
23 # it under the terms of the GNU General Public License as published by
24@@ -41,18 +41,10 @@
25 def get_stack(self, content):
26 return config.MemoryStack(content.encode('utf-8'))
27
28- def test_default_raises_value_error(self):
29- stack = self.get_stack("")
30- self.overrideAttr(_urllib2_wrappers, "DEFAULT_CA_PATH",
31- "/i-do-not-exist")
32- self.assertRaises(ValueError, stack.get, 'ssl.ca_certs')
33-
34 def test_default_exists(self):
35- self.build_tree(['cacerts.pem'])
36+ """Check that the default we provide exists for the tested platform."""
37 stack = self.get_stack("")
38- path = os.path.join(self.test_dir, "cacerts.pem")
39- self.overrideAttr(_urllib2_wrappers, "DEFAULT_CA_PATH", path)
40- self.assertEquals(path, stack.get('ssl.ca_certs'))
41+ self.assertPathExists(stack.get('ssl.ca_certs'))
42
43 def test_specified(self):
44 self.build_tree(['cacerts.pem'])
45@@ -61,14 +53,15 @@
46 self.assertEquals(path, stack.get('ssl.ca_certs'))
47
48 def test_specified_doesnt_exist(self):
49- path = os.path.join(self.test_dir, "nonexisting.pem")
50- stack = self.get_stack("ssl.ca_certs = %s\n" % path)
51+ stack = self.get_stack('')
52+ # Disable the default value mechanism to force the behavior we want
53+ self.overrideAttr(_urllib2_wrappers.opt_ssl_ca_certs, 'default',
54+ os.path.join(self.test_dir, u"nonexisting.pem"))
55 self.warnings = []
56 def warning(*args):
57 self.warnings.append(args[0] % args[1:])
58 self.overrideAttr(trace, 'warning', warning)
59- self.assertEquals(_urllib2_wrappers.DEFAULT_CA_PATH,
60- stack.get('ssl.ca_certs'))
61+ self.assertEquals(None, stack.get('ssl.ca_certs'))
62 self.assertLength(1, self.warnings)
63 self.assertContainsRe(self.warnings[0],
64 "is not valid for \"ssl.ca_certs\"")
65@@ -83,8 +76,6 @@
66 def test_from_string(self):
67 stack = config.MemoryStack("ssl.cert_reqs = none\n")
68 self.assertEquals(ssl.CERT_NONE, stack.get("ssl.cert_reqs"))
69- stack = config.MemoryStack("ssl.cert_reqs = optional\n")
70- self.assertEquals(ssl.CERT_OPTIONAL, stack.get("ssl.cert_reqs"))
71 stack = config.MemoryStack("ssl.cert_reqs = required\n")
72 self.assertEquals(ssl.CERT_REQUIRED, stack.get("ssl.cert_reqs"))
73 stack = config.MemoryStack("ssl.cert_reqs = invalid\n")
74
75=== modified file 'bzrlib/transport/http/_urllib2_wrappers.py'
76--- bzrlib/transport/http/_urllib2_wrappers.py 2012-01-20 09:19:14 +0000
77+++ bzrlib/transport/http/_urllib2_wrappers.py 2012-01-31 17:04:31 +0000
78@@ -1,4 +1,4 @@
79-# Copyright (C) 2006-2011 Canonical Ltd
80+# Copyright (C) 2006-2012 Canonical Ltd
81 #
82 # This program is free software; you can redistribute it and/or modify
83 # it under the terms of the GNU General Public License as published by
84@@ -74,14 +74,36 @@
85 import ssl
86 """)
87
88-DEFAULT_CA_PATH = u"/etc/ssl/certs/ca-certificates.crt"
89
90+# Note for packagers: if there is no package providing certs for your platform,
91+# the curl project produces http://curl.haxx.se/ca/cacert.pem weekly.
92+_ssl_ca_certs_known_locations = [
93+ u'/etc/ssl/certs/ca-certificates.crt', # Ubuntu/debian/gentoo
94+ u'/etc/pki/tls/certs/ca-bundle.crt', # Fedora/CentOS/RH
95+ u'/etc/ssl/ca-bundle.pem', # OpenSuse
96+ u'/etc/ssl/cert.pem', # OpenSuse
97+ u"/usr/local/share/certs/ca-root-nss.crt", # FreeBSD
98+ # XXX: Needs checking, can't trust the interweb ;) -- vila 2012-01-25
99+ u'/etc/openssl/certs/ca-certificates.crt', # Solaris
100+ ]
101
102 def default_ca_certs():
103- if not os.path.exists(DEFAULT_CA_PATH):
104- raise ValueError("default ca certs path %s does not exist" %
105- DEFAULT_CA_PATH)
106- return DEFAULT_CA_PATH
107+ if sys.platform == 'win32':
108+ return os.path.join(os.path.dirname(sys.executable), u"ca_bundle.crt")
109+ elif sys.platform == 'darwin':
110+ # FIXME: Needs some default value for osx, waiting for osx installers
111+ # guys feedback -- vila 2012-01-25
112+ pass
113+ else:
114+ # Try known locations for friendly OSes providing the root certificates
115+ # without making them hard to use for any https client.
116+ for path in _ssl_ca_certs_known_locations:
117+ if os.path.exists(path):
118+ # First found wins
119+ return path
120+ # A default path that makes sense and will be mentioned in the error
121+ # presented to the user, even if not correct for all platforms
122+ return _ssl_ca_certs_known_locations[0]
123
124
125 def ca_certs_from_store(path):
126@@ -90,16 +112,11 @@
127 return path
128
129
130-def default_cert_reqs():
131- return u"required"
132-
133-
134 def cert_reqs_from_store(unicode_str):
135 import ssl
136 try:
137 return {
138 "required": ssl.CERT_REQUIRED,
139- "optional": ssl.CERT_OPTIONAL,
140 "none": ssl.CERT_NONE
141 }[unicode_str]
142 except KeyError:
143@@ -112,10 +129,15 @@
144 invalid='warning',
145 help="""\
146 Path to certification authority certificates to trust.
147+
148+This should be a valid path to a bundle containing all root Certificate
149+Authorities used to verify an https server certificate.
150+
151+Use ssl.cert_reqs=none to disable certificate verification.
152 """)
153
154 opt_ssl_cert_reqs = config.Option('ssl.cert_reqs',
155- default=default_cert_reqs,
156+ default=u"required",
157 from_unicode=cert_reqs_from_store,
158 invalid='error',
159 help="""\
160@@ -123,8 +145,7 @@
161
162 Possible values:
163 * none: Certificates ignored
164- * optional: Certificates not required, but validated if provided
165- * required: Certificates required, and validated
166+ * required: Certificates required and validated
167 """)
168
169 checked_kerberos = False
170@@ -448,34 +469,34 @@
171 def connect_to_origin(self):
172 # FIXME JRV 2011-12-18: Use location config here?
173 config_stack = config.GlobalStack()
174- if self.ca_certs is None:
175- ca_certs = config_stack.get('ssl.ca_certs')
176- else:
177- ca_certs = self.ca_certs
178 cert_reqs = config_stack.get('ssl.cert_reqs')
179 if cert_reqs == ssl.CERT_NONE:
180- trace.warning("not checking SSL certificates for %s: %d",
181+ trace.warning("Not checking SSL certificate for %s: %d",
182 self.host, self.port)
183+ ca_certs = None
184 else:
185+ if self.ca_certs is None:
186+ ca_certs = config_stack.get('ssl.ca_certs')
187+ else:
188+ ca_certs = self.ca_certs
189 if ca_certs is None:
190 trace.warning(
191- "no valid trusted SSL CA certificates file set. See "
192+ "No valid trusted SSL CA certificates file set. See "
193 "'bzr help ssl.ca_certs' for more information on setting "
194- "trusted CA's.")
195+ "trusted CAs.")
196 try:
197 ssl_sock = ssl.wrap_socket(self.sock, self.key_file, self.cert_file,
198 cert_reqs=cert_reqs, ca_certs=ca_certs)
199 except ssl.SSLError, e:
200- if e.errno != ssl.SSL_ERROR_SSL:
201- raise
202 trace.note(
203- "To disable SSL certificate verification, use "
204- "-Ossl.cert_reqs=none. See ``bzr help ssl.ca_certs`` for "
205- "more information on specifying trusted CA certificates.")
206+ "\n"
207+ "See `bzr help ssl.ca_certs` for how to specify trusted CA"
208+ "certificates.\n"
209+ "Pass -Ossl.cert_reqs=none to disable certificate "
210+ "verification entirely.\n")
211 raise
212- peer_cert = ssl_sock.getpeercert()
213- if (cert_reqs == ssl.CERT_REQUIRED or
214- (cert_reqs == ssl.CERT_OPTIONAL and peer_cert)):
215+ if cert_reqs == ssl.CERT_REQUIRED:
216+ peer_cert = ssl_sock.getpeercert()
217 match_hostname(peer_cert, self.host)
218
219 # Wrap the ssl socket before anybody use it
220
221=== modified file 'doc/en/release-notes/bzr-2.5.txt'
222--- doc/en/release-notes/bzr-2.5.txt 2012-01-31 15:43:17 +0000
223+++ doc/en/release-notes/bzr-2.5.txt 2012-01-31 17:04:31 +0000
224@@ -56,12 +56,19 @@
225 * ``bzr branch`` now fetches revisions when branching into an empty
226 control directory. (Jelmer Vernooij, #905594)
227
228+<<<<<<< TREE
229 * ``bzr branch`` generates correct target branch locations again if not
230 specified. (Jelmer Vernooij, #919218)
231
232 * ``bzr send`` works on treeless branches again.
233 (Jelmer Vernooij, #921591)
234
235+=======
236+* A sane default is provided for ``ssl.ca_certs`` which should points to the
237+ Certificate Authority bundle for supported platforms.
238+ (Vincent Ladeuil, #920455)
239+
240+>>>>>>> MERGE-SOURCE
241 * Support scripts that don't call bzrlib.initialize() but still call run_bzr().
242 (Vincent Ladeuil, #917733)
243

Subscribers

People subscribed via source and target branches