Merge lp:~jelmer/bzr/urllib-verifies-ssl-certs into lp:bzr
- urllib-verifies-ssl-certs
- Merge into bzr.dev
Status: | Merged | ||||||||
---|---|---|---|---|---|---|---|---|---|
Merge reported by: | Vincent Ladeuil | ||||||||
Merged at revision: | not available | ||||||||
Proposed branch: | lp:~jelmer/bzr/urllib-verifies-ssl-certs | ||||||||
Merge into: | lp:bzr | ||||||||
Diff against target: |
441 lines (+293/-18) 9 files modified
bzrlib/config.py (+7/-0) bzrlib/errors.py (+8/-0) bzrlib/tests/__init__.py (+1/-0) bzrlib/tests/test_http.py (+0/-1) bzrlib/tests/test_https_urllib.py (+109/-0) bzrlib/transport/__init__.py (+3/-3) bzrlib/transport/http/_urllib2_wrappers.py (+142/-14) doc/en/release-notes/bzr-2.5.txt (+6/-0) doc/en/whats-new/whats-new-in-2.5.txt (+17/-0) |
||||||||
To merge this branch: | bzr merge lp:~jelmer/bzr/urllib-verifies-ssl-certs | ||||||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Vincent Ladeuil | Needs Fixing | ||
Martin Packman (community) | Approve | ||
Review via email: mp+87380@code.launchpad.net |
This proposal supersedes a proposal from 2011-12-20.
Commit message
Support verification of SSL certificates for the urllib HTTPS backend.
Description of the change
Add support for verifying SSL certificates when using urllib.
Martin Pool (mbp) wrote : Posted in a previous version of this proposal | # |
Vincent Ladeuil (vila) wrote : Posted in a previous version of this proposal | # |
> Add support for verifying SSL certificates when using urllib.
\o/
>
> This branch does actually work, but it's got some open ends:
>
> * The location with the CAs to trust differs per system. Rather than hardcoding
> a list, I think we should have a configuration option that allows the user
> to specify what CA list they want to verify against.
pycurl already handles that for windows but getting it right in the general
case... sounds like a packaging issue :-/
>
> This option could then also be pointed at a user-specific list
> (rather than a system wide list). And packagers can put in the default
> for their distribution.
>
> Where do I obtain this option from, which config stack ?
I'd go with global to start with specifying the default value at
registration time if possible.
Arguably it's a system-wide config file that should be used, making packager
life easier.
>
> * We probably want some way to specify that self-signed certificates are ok.
> Probably also a new option? We should probably still warn the user
> when a self-signed certificate is being trusted.
authentication.conf is the right place for that and the option is even
documented but not implemented ;)
>
> * There are no additional tests. There probably should be some, at least for
> e.g. the match_hostname helper.
Yup. There are already https tests for pycurl that should be activated now
(<broken record> hey, did I mention the 'scenarios' attribute is cool for
that ? </broken record> ;)
>
> * I have the feeling that I'm missing something. This was way too easy.
> What is it?
Hehe.
The last time I worked on this area we were still supporting python-2.4/2.5
where https wasn't supported. Now, you also found the relevant code in
python-3.2 so... that certainly simplify things *a lot* :)
This wasn't a full review but as I understand it you weren't requesting one
either ;)
As for tests, if you look at the pycurl ones you'll certainly find that most
of the needed infrastructure is there to build certificates for special
purposes (self-signed, obsolete, etc) and easily setup an https server.
99 + SSLSocket.
100 + are mostly followed, but IP addresses are not accepted for *hostname*.
Hmm, IP addresses not accepted ? I bet our tests won't like that much.
pycurl itself was not open enough to test for these weird certificates AFAIR
but it's also a robust implementation so I wasn't concerned, but if we make
urllib the default one, we have to make sure we're making the right checks.
I'll need to dig a bit more to find back the relevant discussions about this
topic.
Also, defaulting to urllib is fine but the documentation probably needs to
be updated (created even ?) to make it clear that we're not defaulting to
pycurl. We had issues in the past (including failures to check lp
certificate) which had led to the actual defaults (urllib for http, pycurl
for https), let's make the switch crystal clear.
141 def connect_
142 - ssl_sock = _ssl_wrap_
143 + ssl_sock = ssl.wrap_
144 + ...
Martin Packman (gz) wrote : Posted in a previous version of this proposal | # |
I think there's general agreement this is great, and with the reqirement of Python 2.6 something that's now acheivable and should probably get into the next bzr release.
It absolutely needs some kind of automated testing though, and likely an option for disabling checks, I've used `curl -k` and `wget --no-check-
Martin Packman (gz) wrote : Posted in a previous version of this proposal | # |
Summarising from earlier rambling chat:
* The error shown when a certificate can't be checked because no local store is found needs to spell out the `-Ossl.
* Perhaps the error shown when a self-signed certificate doesn't validate against the local store also wants some love.
* An entry in doc/en/
The mercurial wiki page linked earlier is a useful comparison:
<http://
Jelmer Vernooij (jelmer) wrote : Posted in a previous version of this proposal | # |
> Summarising from earlier rambling chat:
>
> * The error shown when a certificate can't be checked because no local store
> is found needs to spell out the `-Ossl.
> the old insecure behaviour, and maybe a pointer to more help.
> * Perhaps the error shown when a self-signed certificate doesn't validate
> against the local store also wants some love.
> * An entry in doc/en/
> the user-facing aspects of this change, as it's a nice improvement but some
> people may need to change their configuration on upgrading.
Done.
Ironically I can't actually run "bzr selftest -s bt.test_http" on my machine because pycurl triggers segfaults (not just on this branch), but the changes are mostly documentation anyway.
Martin Packman (gz) wrote : | # |
Added what's new text is great, and the new logic for printing helpful messages seems good. Some of these messages could probably do with gettext() added, but that can wait for another branch when the rest of the module can be looked over too.
- if cert_reqs == "none":
+ if cert_reqs == ssl.CERT_NONE:
Seen in the interdiff for the last change, was this broken before, and did a test fail because of it?
Jelmer Vernooij (jelmer) wrote : | # |
sent to pqm by email
Jelmer Vernooij (jelmer) wrote : | # |
sent to pqm by email
Jelmer Vernooij (jelmer) wrote : | # |
sent to pqm by email
Jelmer Vernooij (jelmer) wrote : | # |
> Added what's new text is great, and the new logic for printing helpful
> messages seems good. Some of these messages could probably do with gettext()
> added, but that can wait for another branch when the rest of the module can be
> looked over too.
>
> - if cert_reqs == "none":
> + if cert_reqs == ssl.CERT_NONE:
>
> Seen in the interdiff for the last change, was this broken before, and did a
> test fail because of it?
Yeah, it was indeed broken - and I hadn't noticed since we don't have a test for it. There is an open bug about doing more testing with SSL certificates.
Vincent Ladeuil (vila) wrote : | # |
Will look into it asap but at least the news needs to be fixed.
Preview Diff
1 | === modified file 'bzrlib/config.py' | |||
2 | --- bzrlib/config.py 2012-01-05 14:10:32 +0000 | |||
3 | +++ bzrlib/config.py 2012-01-07 00:08:23 +0000 | |||
4 | @@ -2867,6 +2867,13 @@ | |||
5 | 2867 | Option('submit_to', | 2867 | Option('submit_to', |
6 | 2868 | help='''Where submissions from this branch are mailed to.''')) | 2868 | help='''Where submissions from this branch are mailed to.''')) |
7 | 2869 | 2869 | ||
8 | 2870 | option_registry.register_lazy('ssl.ca_certs', | ||
9 | 2871 | 'bzrlib.transport.http._urllib2_wrappers', 'opt_ssl_ca_certs') | ||
10 | 2872 | |||
11 | 2873 | option_registry.register_lazy('ssl.cert_reqs', | ||
12 | 2874 | 'bzrlib.transport.http._urllib2_wrappers', 'opt_ssl_cert_reqs') | ||
13 | 2875 | |||
14 | 2876 | |||
15 | 2870 | 2877 | ||
16 | 2871 | class Section(object): | 2878 | class Section(object): |
17 | 2872 | """A section defines a dict of option name => value. | 2879 | """A section defines a dict of option name => value. |
18 | 2873 | 2880 | ||
19 | === modified file 'bzrlib/errors.py' | |||
20 | --- bzrlib/errors.py 2011-12-22 15:33:16 +0000 | |||
21 | +++ bzrlib/errors.py 2012-01-07 00:08:23 +0000 | |||
22 | @@ -1667,6 +1667,14 @@ | |||
23 | 1667 | TransportError.__init__(self, msg, orig_error=orig_error) | 1667 | TransportError.__init__(self, msg, orig_error=orig_error) |
24 | 1668 | 1668 | ||
25 | 1669 | 1669 | ||
26 | 1670 | class CertificateError(TransportError): | ||
27 | 1671 | |||
28 | 1672 | _fmt = "Certificate error: %(error)s" | ||
29 | 1673 | |||
30 | 1674 | def __init__(self, error): | ||
31 | 1675 | self.error = error | ||
32 | 1676 | |||
33 | 1677 | |||
34 | 1670 | class InvalidHttpRange(InvalidHttpResponse): | 1678 | class InvalidHttpRange(InvalidHttpResponse): |
35 | 1671 | 1679 | ||
36 | 1672 | _fmt = "Invalid http range %(range)r for %(path)s: %(msg)s" | 1680 | _fmt = "Invalid http range %(range)r for %(path)s: %(msg)s" |
37 | 1673 | 1681 | ||
38 | === modified file 'bzrlib/tests/__init__.py' | |||
39 | --- bzrlib/tests/__init__.py 2011-12-24 10:10:59 +0000 | |||
40 | +++ bzrlib/tests/__init__.py 2012-01-07 00:08:23 +0000 | |||
41 | @@ -4002,6 +4002,7 @@ | |||
42 | 4002 | 'bzrlib.tests.test_http', | 4002 | 'bzrlib.tests.test_http', |
43 | 4003 | 'bzrlib.tests.test_http_response', | 4003 | 'bzrlib.tests.test_http_response', |
44 | 4004 | 'bzrlib.tests.test_https_ca_bundle', | 4004 | 'bzrlib.tests.test_https_ca_bundle', |
45 | 4005 | 'bzrlib.tests.test_https_urllib', | ||
46 | 4005 | 'bzrlib.tests.test_i18n', | 4006 | 'bzrlib.tests.test_i18n', |
47 | 4006 | 'bzrlib.tests.test_identitymap', | 4007 | 'bzrlib.tests.test_identitymap', |
48 | 4007 | 'bzrlib.tests.test_ignores', | 4008 | 'bzrlib.tests.test_ignores', |
49 | 4008 | 4009 | ||
50 | === modified file 'bzrlib/tests/test_http.py' | |||
51 | --- bzrlib/tests/test_http.py 2011-10-12 16:00:13 +0000 | |||
52 | +++ bzrlib/tests/test_http.py 2012-01-07 00:08:23 +0000 | |||
53 | @@ -32,7 +32,6 @@ | |||
54 | 32 | import bzrlib | 32 | import bzrlib |
55 | 33 | from bzrlib import ( | 33 | from bzrlib import ( |
56 | 34 | bzrdir, | 34 | bzrdir, |
57 | 35 | cethread, | ||
58 | 36 | config, | 35 | config, |
59 | 37 | debug, | 36 | debug, |
60 | 38 | errors, | 37 | errors, |
61 | 39 | 38 | ||
62 | === added file 'bzrlib/tests/test_https_urllib.py' | |||
63 | --- bzrlib/tests/test_https_urllib.py 1970-01-01 00:00:00 +0000 | |||
64 | +++ bzrlib/tests/test_https_urllib.py 2012-01-07 00:08:23 +0000 | |||
65 | @@ -0,0 +1,109 @@ | |||
66 | 1 | # Copyright (C) 2011 Canonical Ltd | ||
67 | 2 | # | ||
68 | 3 | # This program is free software; you can redistribute it and/or modify | ||
69 | 4 | # it under the terms of the GNU General Public License as published by | ||
70 | 5 | # the Free Software Foundation; either version 2 of the License, or | ||
71 | 6 | # (at your option) any later version. | ||
72 | 7 | # | ||
73 | 8 | # This program is distributed in the hope that it will be useful, | ||
74 | 9 | # but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
75 | 10 | # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | ||
76 | 11 | # GNU General Public License for more details. | ||
77 | 12 | # | ||
78 | 13 | # You should have received a copy of the GNU General Public License | ||
79 | 14 | # along with this program; if not, write to the Free Software | ||
80 | 15 | # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA | ||
81 | 16 | |||
82 | 17 | """Tests for the SSL support in the urllib HTTP transport. | ||
83 | 18 | |||
84 | 19 | """ | ||
85 | 20 | |||
86 | 21 | import ssl | ||
87 | 22 | |||
88 | 23 | from bzrlib import trace | ||
89 | 24 | from bzrlib.errors import ( | ||
90 | 25 | CertificateError, | ||
91 | 26 | ConfigOptionValueError, | ||
92 | 27 | ) | ||
93 | 28 | from bzrlib.config import ( | ||
94 | 29 | IniFileStore, | ||
95 | 30 | Stack, | ||
96 | 31 | ) | ||
97 | 32 | import os | ||
98 | 33 | from bzrlib.tests import ( | ||
99 | 34 | TestCase, | ||
100 | 35 | TestCaseInTempDir, | ||
101 | 36 | ) | ||
102 | 37 | from bzrlib.transport.http import _urllib2_wrappers | ||
103 | 38 | |||
104 | 39 | |||
105 | 40 | def stack_from_string(text): | ||
106 | 41 | store = IniFileStore() | ||
107 | 42 | store._load_from_string(text) | ||
108 | 43 | return Stack([store.get_sections]) | ||
109 | 44 | |||
110 | 45 | |||
111 | 46 | class CaCertsConfigTests(TestCaseInTempDir): | ||
112 | 47 | |||
113 | 48 | def test_default_raises_value_error(self): | ||
114 | 49 | stack = stack_from_string("") | ||
115 | 50 | self.overrideAttr(_urllib2_wrappers, "DEFAULT_CA_PATH", | ||
116 | 51 | "/i-do-not-exist") | ||
117 | 52 | self.assertRaises(ValueError, stack.get, 'ssl.ca_certs') | ||
118 | 53 | |||
119 | 54 | def test_default_exists(self): | ||
120 | 55 | self.build_tree(['cacerts.pem']) | ||
121 | 56 | stack = stack_from_string("") | ||
122 | 57 | path = os.path.join(self.test_dir, "cacerts.pem") | ||
123 | 58 | self.overrideAttr(_urllib2_wrappers, "DEFAULT_CA_PATH", path) | ||
124 | 59 | self.assertEquals(path, stack.get('ssl.ca_certs')) | ||
125 | 60 | |||
126 | 61 | def test_specified(self): | ||
127 | 62 | self.build_tree(['cacerts.pem']) | ||
128 | 63 | path = os.path.join(self.test_dir, "cacerts.pem") | ||
129 | 64 | stack = stack_from_string("ssl.ca_certs = %s\n" % path) | ||
130 | 65 | self.assertEquals(path, stack.get('ssl.ca_certs')) | ||
131 | 66 | |||
132 | 67 | def test_specified_doesnt_exist(self): | ||
133 | 68 | path = os.path.join(self.test_dir, "nonexisting.pem") | ||
134 | 69 | stack = stack_from_string("ssl.ca_certs = %s\n" % path) | ||
135 | 70 | self.warnings = [] | ||
136 | 71 | def warning(*args): | ||
137 | 72 | self.warnings.append(args[0] % args[1:]) | ||
138 | 73 | self.overrideAttr(trace, 'warning', warning) | ||
139 | 74 | self.assertEquals(_urllib2_wrappers.DEFAULT_CA_PATH, stack.get('ssl.ca_certs')) | ||
140 | 75 | self.assertLength(1, self.warnings) | ||
141 | 76 | self.assertContainsRe(self.warnings[0], "is not valid for \"ssl.ca_certs\"") | ||
142 | 77 | |||
143 | 78 | |||
144 | 79 | class CertReqsConfigTests(TestCaseInTempDir): | ||
145 | 80 | |||
146 | 81 | def test_default(self): | ||
147 | 82 | stack = stack_from_string("") | ||
148 | 83 | self.assertEquals(ssl.CERT_REQUIRED, stack.get("ssl.cert_reqs")) | ||
149 | 84 | |||
150 | 85 | def test_from_string(self): | ||
151 | 86 | stack = stack_from_string("ssl.cert_reqs = none\n") | ||
152 | 87 | self.assertEquals(ssl.CERT_NONE, stack.get("ssl.cert_reqs")) | ||
153 | 88 | stack = stack_from_string("ssl.cert_reqs = optional\n") | ||
154 | 89 | self.assertEquals(ssl.CERT_OPTIONAL, stack.get("ssl.cert_reqs")) | ||
155 | 90 | stack = stack_from_string("ssl.cert_reqs = required\n") | ||
156 | 91 | self.assertEquals(ssl.CERT_REQUIRED, stack.get("ssl.cert_reqs")) | ||
157 | 92 | stack = stack_from_string("ssl.cert_reqs = invalid\n") | ||
158 | 93 | self.assertRaises(ConfigOptionValueError, stack.get, "ssl.cert_reqs") | ||
159 | 94 | |||
160 | 95 | |||
161 | 96 | class MatchHostnameTests(TestCase): | ||
162 | 97 | |||
163 | 98 | def test_no_certificate(self): | ||
164 | 99 | self.assertRaises(ValueError, _urllib2_wrappers.match_hostname({})) | ||
165 | 100 | |||
166 | 101 | def test_no_valid_attributes(self): | ||
167 | 102 | self.assertRaises(CertificateError, | ||
168 | 103 | _urllib2_wrappers.match_hostname({"Problem": "Solved"})) | ||
169 | 104 | |||
170 | 105 | def test_common_name(self): | ||
171 | 106 | cert = {'subject': ((('commonName', 'example.com'),),)} | ||
172 | 107 | self.assertIs(None, _urllib2_wrappers.match_hostname(cert, "example.com")) | ||
173 | 108 | self.assertRaises(CertificateError, _urllib2_wrappers.match_hostname, | ||
174 | 109 | cert, "example.org") | ||
175 | 0 | 110 | ||
176 | === modified file 'bzrlib/transport/__init__.py' | |||
177 | --- bzrlib/transport/__init__.py 2011-12-24 10:10:59 +0000 | |||
178 | +++ bzrlib/transport/__init__.py 2012-01-07 00:08:23 +0000 | |||
179 | @@ -1782,15 +1782,15 @@ | |||
180 | 1782 | help="Read-only access of branches exported on the web.") | 1782 | help="Read-only access of branches exported on the web.") |
181 | 1783 | register_transport_proto('https://', | 1783 | register_transport_proto('https://', |
182 | 1784 | help="Read-only access of branches exported on the web using SSL.") | 1784 | help="Read-only access of branches exported on the web using SSL.") |
184 | 1785 | # The default http implementation is urllib, but https is pycurl if available | 1785 | # The default http implementation is urllib |
185 | 1786 | register_lazy_transport('http://', 'bzrlib.transport.http._pycurl', | 1786 | register_lazy_transport('http://', 'bzrlib.transport.http._pycurl', |
186 | 1787 | 'PyCurlTransport') | 1787 | 'PyCurlTransport') |
187 | 1788 | register_lazy_transport('https://', 'bzrlib.transport.http._pycurl', | ||
188 | 1789 | 'PyCurlTransport') | ||
189 | 1788 | register_lazy_transport('http://', 'bzrlib.transport.http._urllib', | 1790 | register_lazy_transport('http://', 'bzrlib.transport.http._urllib', |
190 | 1789 | 'HttpTransport_urllib') | 1791 | 'HttpTransport_urllib') |
191 | 1790 | register_lazy_transport('https://', 'bzrlib.transport.http._urllib', | 1792 | register_lazy_transport('https://', 'bzrlib.transport.http._urllib', |
192 | 1791 | 'HttpTransport_urllib') | 1793 | 'HttpTransport_urllib') |
193 | 1792 | register_lazy_transport('https://', 'bzrlib.transport.http._pycurl', | ||
194 | 1793 | 'PyCurlTransport') | ||
195 | 1794 | 1794 | ||
196 | 1795 | register_transport_proto('ftp://', help="Access using passive FTP.") | 1795 | register_transport_proto('ftp://', help="Access using passive FTP.") |
197 | 1796 | register_lazy_transport('ftp://', 'bzrlib.transport.ftp', 'FtpTransport') | 1796 | register_lazy_transport('ftp://', 'bzrlib.transport.ftp', 'FtpTransport') |
198 | 1797 | 1797 | ||
199 | === modified file 'bzrlib/transport/http/_urllib2_wrappers.py' | |||
200 | --- bzrlib/transport/http/_urllib2_wrappers.py 2011-12-19 13:23:58 +0000 | |||
201 | +++ bzrlib/transport/http/_urllib2_wrappers.py 2012-01-07 00:08:23 +0000 | |||
202 | @@ -14,7 +14,7 @@ | |||
203 | 14 | # along with this program; if not, write to the Free Software | 14 | # along with this program; if not, write to the Free Software |
204 | 15 | # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA | 15 | # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA |
205 | 16 | 16 | ||
207 | 17 | """Implementaion of urllib2 tailored to bzr needs | 17 | """Implementation of urllib2 tailored to bzr needs |
208 | 18 | 18 | ||
209 | 19 | This file complements the urllib2 class hierarchy with custom classes. | 19 | This file complements the urllib2 class hierarchy with custom classes. |
210 | 20 | 20 | ||
211 | @@ -50,6 +50,7 @@ | |||
212 | 50 | 50 | ||
213 | 51 | import errno | 51 | import errno |
214 | 52 | import httplib | 52 | import httplib |
215 | 53 | import os | ||
216 | 53 | import socket | 54 | import socket |
217 | 54 | import urllib | 55 | import urllib |
218 | 55 | import urllib2 | 56 | import urllib2 |
219 | @@ -63,12 +64,68 @@ | |||
220 | 63 | config, | 64 | config, |
221 | 64 | debug, | 65 | debug, |
222 | 65 | errors, | 66 | errors, |
223 | 67 | lazy_import, | ||
224 | 66 | osutils, | 68 | osutils, |
225 | 67 | trace, | 69 | trace, |
226 | 68 | transport, | 70 | transport, |
227 | 69 | urlutils, | 71 | urlutils, |
228 | 70 | ) | 72 | ) |
230 | 71 | 73 | lazy_import.lazy_import(globals(), """ | |
231 | 74 | import ssl | ||
232 | 75 | """) | ||
233 | 76 | |||
234 | 77 | DEFAULT_CA_PATH = u"/etc/ssl/certs/ca-certificates.crt" | ||
235 | 78 | |||
236 | 79 | |||
237 | 80 | def default_ca_certs(): | ||
238 | 81 | if not os.path.exists(DEFAULT_CA_PATH): | ||
239 | 82 | raise ValueError("default ca certs path %s does not exist" % | ||
240 | 83 | DEFAULT_CA_PATH) | ||
241 | 84 | return DEFAULT_CA_PATH | ||
242 | 85 | |||
243 | 86 | |||
244 | 87 | def ca_certs_from_store(path): | ||
245 | 88 | if not os.path.exists(path): | ||
246 | 89 | raise ValueError("ca certs path %s does not exist" % path) | ||
247 | 90 | return path | ||
248 | 91 | |||
249 | 92 | |||
250 | 93 | def default_cert_reqs(): | ||
251 | 94 | return u"required" | ||
252 | 95 | |||
253 | 96 | |||
254 | 97 | def cert_reqs_from_store(unicode_str): | ||
255 | 98 | import ssl | ||
256 | 99 | try: | ||
257 | 100 | return { | ||
258 | 101 | "required": ssl.CERT_REQUIRED, | ||
259 | 102 | "optional": ssl.CERT_OPTIONAL, | ||
260 | 103 | "none": ssl.CERT_NONE | ||
261 | 104 | }[unicode_str] | ||
262 | 105 | except KeyError: | ||
263 | 106 | raise ValueError("invalid value %s" % unicode_str) | ||
264 | 107 | |||
265 | 108 | |||
266 | 109 | opt_ssl_ca_certs = config.Option('ssl.ca_certs', | ||
267 | 110 | from_unicode=ca_certs_from_store, | ||
268 | 111 | default=default_ca_certs, | ||
269 | 112 | invalid='warning', | ||
270 | 113 | help="""\ | ||
271 | 114 | Path to certification authority certificates to trust. | ||
272 | 115 | """) | ||
273 | 116 | |||
274 | 117 | opt_ssl_cert_reqs = config.Option('ssl.cert_reqs', | ||
275 | 118 | default=default_cert_reqs, | ||
276 | 119 | from_unicode=cert_reqs_from_store, | ||
277 | 120 | invalid='error', | ||
278 | 121 | help="""\ | ||
279 | 122 | Whether to require a certificate from the remote side. (default:required) | ||
280 | 123 | |||
281 | 124 | Possible values: | ||
282 | 125 | * none: certificates ignored | ||
283 | 126 | * optional: Certificates not required, but validated if provided | ||
284 | 127 | * required: Certificates required, and validated | ||
285 | 128 | """) | ||
286 | 72 | 129 | ||
287 | 73 | checked_kerberos = False | 130 | checked_kerberos = False |
288 | 74 | kerberos = None | 131 | kerberos = None |
289 | @@ -312,17 +369,60 @@ | |||
290 | 312 | self._wrap_socket_for_reporting(self.sock) | 369 | self._wrap_socket_for_reporting(self.sock) |
291 | 313 | 370 | ||
292 | 314 | 371 | ||
304 | 315 | # Build the appropriate socket wrapper for ssl | 372 | # These two methods were imported from Python 3.2's ssl module |
305 | 316 | try: | 373 | |
306 | 317 | # python 2.6 introduced a better ssl package | 374 | def _dnsname_to_pat(dn): |
307 | 318 | import ssl | 375 | pats = [] |
308 | 319 | _ssl_wrap_socket = ssl.wrap_socket | 376 | for frag in dn.split(r'.'): |
309 | 320 | except ImportError: | 377 | if frag == '*': |
310 | 321 | # python versions prior to 2.6 don't have ssl and ssl.wrap_socket instead | 378 | # When '*' is a fragment by itself, it matches a non-empty dotless |
311 | 322 | # they use httplib.FakeSocket | 379 | # fragment. |
312 | 323 | def _ssl_wrap_socket(sock, key_file, cert_file): | 380 | pats.append('[^.]+') |
313 | 324 | ssl_sock = socket.ssl(sock, key_file, cert_file) | 381 | else: |
314 | 325 | return httplib.FakeSocket(sock, ssl_sock) | 382 | # Otherwise, '*' matches any dotless fragment. |
315 | 383 | frag = re.escape(frag) | ||
316 | 384 | pats.append(frag.replace(r'\*', '[^.]*')) | ||
317 | 385 | return re.compile(r'\A' + r'\.'.join(pats) + r'\Z', re.IGNORECASE) | ||
318 | 386 | |||
319 | 387 | |||
320 | 388 | def match_hostname(cert, hostname): | ||
321 | 389 | """Verify that *cert* (in decoded format as returned by | ||
322 | 390 | SSLSocket.getpeercert()) matches the *hostname*. RFC 2818 rules | ||
323 | 391 | are mostly followed, but IP addresses are not accepted for *hostname*. | ||
324 | 392 | |||
325 | 393 | CertificateError is raised on failure. On success, the function | ||
326 | 394 | returns nothing. | ||
327 | 395 | """ | ||
328 | 396 | if not cert: | ||
329 | 397 | raise ValueError("empty or no certificate") | ||
330 | 398 | dnsnames = [] | ||
331 | 399 | san = cert.get('subjectAltName', ()) | ||
332 | 400 | for key, value in san: | ||
333 | 401 | if key == 'DNS': | ||
334 | 402 | if _dnsname_to_pat(value).match(hostname): | ||
335 | 403 | return | ||
336 | 404 | dnsnames.append(value) | ||
337 | 405 | if not san: | ||
338 | 406 | # The subject is only checked when subjectAltName is empty | ||
339 | 407 | for sub in cert.get('subject', ()): | ||
340 | 408 | for key, value in sub: | ||
341 | 409 | # XXX according to RFC 2818, the most specific Common Name | ||
342 | 410 | # must be used. | ||
343 | 411 | if key == 'commonName': | ||
344 | 412 | if _dnsname_to_pat(value).match(hostname): | ||
345 | 413 | return | ||
346 | 414 | dnsnames.append(value) | ||
347 | 415 | if len(dnsnames) > 1: | ||
348 | 416 | raise errors.CertificateError("hostname %r " | ||
349 | 417 | "doesn't match either of %s" | ||
350 | 418 | % (hostname, ', '.join(map(repr, dnsnames)))) | ||
351 | 419 | elif len(dnsnames) == 1: | ||
352 | 420 | raise errors.CertificateError("hostname %r " | ||
353 | 421 | "doesn't match %r" | ||
354 | 422 | % (hostname, dnsnames[0])) | ||
355 | 423 | else: | ||
356 | 424 | raise errors.CertificateError("no appropriate commonName or " | ||
357 | 425 | "subjectAltName fields were found") | ||
358 | 326 | 426 | ||
359 | 327 | 427 | ||
360 | 328 | class HTTPSConnection(AbstractHTTPConnection, httplib.HTTPSConnection): | 428 | class HTTPSConnection(AbstractHTTPConnection, httplib.HTTPSConnection): |
361 | @@ -345,7 +445,35 @@ | |||
362 | 345 | self.connect_to_origin() | 445 | self.connect_to_origin() |
363 | 346 | 446 | ||
364 | 347 | def connect_to_origin(self): | 447 | def connect_to_origin(self): |
366 | 348 | ssl_sock = _ssl_wrap_socket(self.sock, self.key_file, self.cert_file) | 448 | # FIXME JRV 2011-12-18: Use location config here? |
367 | 449 | config_stack = config.GlobalStack() | ||
368 | 450 | ca_certs = config_stack.get('ssl.ca_certs') | ||
369 | 451 | cert_reqs = config_stack.get('ssl.cert_reqs') | ||
370 | 452 | if cert_reqs == ssl.CERT_NONE: | ||
371 | 453 | trace.warning("not checking SSL certificates for %s: %d", | ||
372 | 454 | self.host, self.port) | ||
373 | 455 | else: | ||
374 | 456 | if ca_certs is None: | ||
375 | 457 | trace.warning( | ||
376 | 458 | "no valid trusted SSL CA certificates file set. See " | ||
377 | 459 | "'bzr help ssl.ca_certs' for more information on setting " | ||
378 | 460 | "trusted CA's.") | ||
379 | 461 | try: | ||
380 | 462 | ssl_sock = ssl.wrap_socket(self.sock, self.key_file, self.cert_file, | ||
381 | 463 | cert_reqs=cert_reqs, ca_certs=ca_certs) | ||
382 | 464 | except ssl.SSLError, e: | ||
383 | 465 | if e.errno != ssl.SSL_ERROR_SSL: | ||
384 | 466 | raise | ||
385 | 467 | trace.note( | ||
386 | 468 | "To disable SSL certificate verification, use " | ||
387 | 469 | "-Ossl.cert_reqs=none. See ``bzr help ssl.ca_certs`` for " | ||
388 | 470 | "more information on specifying trusted CA certificates.") | ||
389 | 471 | raise | ||
390 | 472 | peer_cert = ssl_sock.getpeercert() | ||
391 | 473 | if (cert_reqs == ssl.CERT_REQUIRED or | ||
392 | 474 | (cert_reqs == ssl.CERT_OPTIONAL and peer_cert)): | ||
393 | 475 | match_hostname(peer_cert, self.host) | ||
394 | 476 | |||
395 | 349 | # Wrap the ssl socket before anybody use it | 477 | # Wrap the ssl socket before anybody use it |
396 | 350 | self._wrap_socket_for_reporting(ssl_sock) | 478 | self._wrap_socket_for_reporting(ssl_sock) |
397 | 351 | 479 | ||
398 | 352 | 480 | ||
399 | === modified file 'doc/en/release-notes/bzr-2.5.txt' | |||
400 | --- doc/en/release-notes/bzr-2.5.txt 2012-01-06 14:09:04 +0000 | |||
401 | +++ doc/en/release-notes/bzr-2.5.txt 2012-01-07 00:08:23 +0000 | |||
402 | @@ -479,6 +479,12 @@ | |||
403 | 479 | to 'line-based'. This replace the previous shelf UI only patch using | 479 | to 'line-based'. This replace the previous shelf UI only patch using |
404 | 480 | ``INSIDE_EMACS``. (Benoît Pierre) | 480 | ``INSIDE_EMACS``. (Benoît Pierre) |
405 | 481 | 481 | ||
406 | 482 | * urllib-based HTTPS client connections now verify the server certificate | ||
407 | 483 | as well as the hostname. (Jelmer Vernooij) | ||
408 | 484 | |||
409 | 485 | * urllib-based HTTPS connections are now the default. | ||
410 | 486 | (Jelmer Vernooij, #125055, #651161) | ||
411 | 487 | |||
412 | 482 | Bug Fixes | 488 | Bug Fixes |
413 | 483 | ********* | 489 | ********* |
414 | 484 | 490 | ||
415 | 485 | 491 | ||
416 | === modified file 'doc/en/whats-new/whats-new-in-2.5.txt' | |||
417 | --- doc/en/whats-new/whats-new-in-2.5.txt 2012-01-03 16:28:43 +0000 | |||
418 | +++ doc/en/whats-new/whats-new-in-2.5.txt 2012-01-07 00:08:23 +0000 | |||
419 | @@ -36,6 +36,23 @@ | |||
420 | 36 | encoding for interacting with the filesystem. This makes creating a working | 36 | encoding for interacting with the filesystem. This makes creating a working |
421 | 37 | tree and commiting to it possible for such branches in most environments. | 37 | tree and commiting to it possible for such branches in most environments. |
422 | 38 | 38 | ||
423 | 39 | SSL Certificate Verification Support in urllib HTTPS backend | ||
424 | 40 | ************************************************************ | ||
425 | 41 | |||
426 | 42 | In previous versions of Bazaar, only one of the two supported HTTPS | ||
427 | 43 | backends, pycurl, supported verification of SSL certificates. This version | ||
428 | 44 | also introduces this support for the urllib backend. | ||
429 | 45 | |||
430 | 46 | Along with this support two new options have been introduced to control | ||
431 | 47 | which CA's are trusted and to what degree server certificates should be | ||
432 | 48 | verified. See ``bzr help ssl.ca_certs`` and ``bzr help ssl.cert_reqs`` | ||
433 | 49 | for more information | ||
434 | 50 | |||
435 | 51 | Users who have previously used the urllib HTTPS backend with servers | ||
436 | 52 | with invalid or untrusted certificates can continue to do so by | ||
437 | 53 | adding the required certificates to the trusted CA certificate file | ||
438 | 54 | (recommended) or by setting the ``ssl.cert_reqs`` option to ``none``. | ||
439 | 55 | |||
440 | 39 | Faster smart server | 56 | Faster smart server |
441 | 40 | ******************* | 57 | ******************* |
442 | 41 | 58 |
On 4 November 2011 14:03, Jelmer Vernooij <email address hidden> wrote: /bugs.launchpad .net/bzr/ +bug/125055 /bugs.launchpad .net/bzr/ +bug/651161 /code.launchpad .net/~jelmer/ bzr/urllib- verifies- ssl-certs/ +merge/ 81227
> Jelmer Vernooij has proposed merging lp:~jelmer/bzr/urllib-verifies-ssl-certs into lp:bzr.
>
> Requested reviews:
> bzr-core (bzr-core)
> Related bugs:
> Bug #125055 in Bazaar: "defaulting to pycurl doesn't make sense anymore"
> https:/
> Bug #651161 in Bazaar: "bzr fails to verify ssl validity in https connections - by default --> as pycurl isn't a dep only a suggestion "
> https:/
>
> For more details, see:
> https:/
>
> Add support for verifying SSL certificates when using urllib.
That's great, thanks.
> This branch does actually work, but it's got some open ends:
>
> * The location with the CAs to trust differs per system. Rather than hardcoding
> a list, I think we should have a configuration option that allows the user
> to specify what CA list they want to verify against.
>
> This option could then also be pointed at a user-specific list
> (rather than a system wide list). And packagers can put in the default
> for their distribution.
>
> Where do I obtain this option from, which config stack ?
[needsfixing] I think the global stack would be a good place to start
- it would seem like a kind of contrived case to want different values
on a smaller scope. We can always change it later.
> * We probably want some way to specify that self-signed certificates are ok.
> Probably also a new option? We should probably still warn the user
> when a self-signed certificate is being trusted.
+1 to a new option and to always warning.
Though perhaps it would be equivalent and easier to just have an
option saying "don't do validation."
> * There are no additional tests. There probably should be some, at least for
> e.g. the match_hostname helper.
It would be nice, if it's not too hard, to actually test against an
untrusted cert, or at least to do monkeypatching to make it look like
we got an invalid cert.
[needsfixing] I'd like a unit test for match_hostname covering the
various cases.
> * I have the feeling that I'm missing something. This was way too easy.
> What is it?
I haven't read the rfc but it looks plausible. You could wait for a
review from vila, who has probably touched this most, or robert who is
an http nerd.
I wonder if this needs a change to Windows or Mac packaging to make
sure the certs are there. Or, possibly we can get them from the
system's keyring in some way.