Merge lp:~vila/bzr/1538480-match-hostname into lp:bzr
- 1538480-match-hostname
- Merge into bzr.dev
Status: | Merged |
---|---|
Approved by: | Richard Wilbur |
Approved revision: | no longer in the source branch. |
Merged at revision: | 6614 |
Proposed branch: | lp:~vila/bzr/1538480-match-hostname |
Merge into: | lp:bzr |
Diff against target: |
364 lines (+75/-127) 4 files modified
bzrlib/errors.py (+1/-9) bzrlib/tests/test_https_urllib.py (+32/-28) bzrlib/transport/http/_urllib2_wrappers.py (+39/-90) doc/en/release-notes/bzr-2.7.txt (+3/-0) |
To merge this branch: | bzr merge lp:~vila/bzr/1538480-match-hostname |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Richard Wilbur | Approve | ||
Review via email: mp+284171@code.launchpad.net |
Commit message
Use ssl.match_hostname instead of our own.
Description of the change
Since match_hostname is now provided, use it instead of carrying an obsolete version.
Richard Wilbur (richard-wilbur) wrote : | # |
Vincent Ladeuil (vila) wrote : | # |
> Vincent, what you've done here is excellent and definitely an improvement over
> the contributed patch, exempli gratia, adding the not_ok function.
>
> What I'm curious about is this patch requires python >= 2.7.9. Do we plan to
> bump our python version requirement from 2.6 to 2.7.9?
Ha, the difficult question :)
So I went to https:/
And I'm seeing various projects dropping support for 2.6 too including testtools that is a requirement for bzr tests.
All in all, I think this means that bzr itself cannot support security fixes for python 2.6.
So I dropped the obsolete copy of python 3.2 (not supported anymore either).
In turns this means that people using a bzr 2.6 AND wanting https security support needs to upgrade to python 2.7.
In other words, we attempted to support cert checking with py2.6 at best as we could, we cannot anymore. This specific feature is controlled via the 'ssl.cert_reqs' config option which can be disabled if needed.
So with this patch, py27 users get better security, py26 users will need to verify their certs by other means and use the option to disable bzr checks but won't be lured into a false sense of security.
Apart from that scenario, py26 is still supported (but not regularly tested to the best of my knowledge).
>
> The other part of the patch checks for match_hostname in the ssl library and,
> if not found, imports it from backports.
I've never heard about a python 'backports' library, may be that's specific to the distribution the patch author is using ?
> 1. Is that a worthwhile exercise in light of python version requirements?
I don't think so, if we need to invest into a porting effort, I'd rather see work around forward porting to py3 than back porting to py2.6 ;)
On the other hand, we may want to officially stop supporting py26 in the future.
> 2. Is backports normally available or would we change our dependencies to get
> it?
Not that I know of.
Richard Wilbur (richard-wilbur) wrote : | # |
Could we then as part of this fix automatically disable the 'ssl.cert_reqs' config option if running under python 2.6 with a warning that the functionality is only available with python 2.x >= 2.7.9 or 3.x >= 3.3?
Vincent Ladeuil (vila) wrote : | # |
> Could we then as part of this fix automatically disable the 'ssl.cert_reqs'
> config option if running under python 2.6 with a warning that the
> functionality is only available with python 2.x >= 2.7.9 or 3.x >= 3.3?
/me facepalms
Of course !
Done.
Richard Wilbur (richard-wilbur) wrote : | # |
Thanks for that last revision. I think a descriptive warning is significantly better than an exception!
+2
Vincent Ladeuil (vila) wrote : | # |
sent to pqm by email
Vincent Ladeuil (vila) wrote : | # |
sent to pqm by email
Preview Diff
1 | === modified file 'bzrlib/errors.py' |
2 | --- bzrlib/errors.py 2013-10-04 09:56:23 +0000 |
3 | +++ bzrlib/errors.py 2016-01-31 13:08:48 +0000 |
4 | @@ -1,4 +1,4 @@ |
5 | -# Copyright (C) 2005-2011 Canonical Ltd |
6 | +# Copyright (C) 2005-2013, 2016 Canonical Ltd |
7 | # |
8 | # This program is free software; you can redistribute it and/or modify |
9 | # it under the terms of the GNU General Public License as published by |
10 | @@ -1686,14 +1686,6 @@ |
11 | TransportError.__init__(self, msg, orig_error=orig_error) |
12 | |
13 | |
14 | -class CertificateError(TransportError): |
15 | - |
16 | - _fmt = "Certificate error: %(error)s" |
17 | - |
18 | - def __init__(self, error): |
19 | - self.error = error |
20 | - |
21 | - |
22 | class InvalidHttpRange(InvalidHttpResponse): |
23 | |
24 | _fmt = "Invalid http range %(range)r for %(path)s: %(msg)s" |
25 | |
26 | === modified file 'bzrlib/tests/test_https_urllib.py' |
27 | --- bzrlib/tests/test_https_urllib.py 2013-05-20 16:38:11 +0000 |
28 | +++ bzrlib/tests/test_https_urllib.py 2016-01-31 13:08:48 +0000 |
29 | @@ -1,4 +1,4 @@ |
30 | -# Copyright (C) 2011,2012 Canonical Ltd |
31 | +# Copyright (C) 2011, 2012, 2013, 2016 Canonical Ltd |
32 | # |
33 | # This program is free software; you can redistribute it and/or modify |
34 | # it under the terms of the GNU General Public License as published by |
35 | @@ -19,24 +19,21 @@ |
36 | """ |
37 | |
38 | import os |
39 | -import ssl |
40 | +import sys |
41 | |
42 | from bzrlib import ( |
43 | config, |
44 | trace, |
45 | - ) |
46 | +) |
47 | from bzrlib.errors import ( |
48 | - CertificateError, |
49 | ConfigOptionValueError, |
50 | - ) |
51 | -from bzrlib.tests import ( |
52 | - TestCase, |
53 | - TestCaseInTempDir, |
54 | - ) |
55 | +) |
56 | +from bzrlib import tests |
57 | from bzrlib.transport.http import _urllib2_wrappers |
58 | - |
59 | - |
60 | -class CaCertsConfigTests(TestCaseInTempDir): |
61 | +from bzrlib.transport.http._urllib2_wrappers import ssl |
62 | + |
63 | + |
64 | +class CaCertsConfigTests(tests.TestCaseInTempDir): |
65 | |
66 | def get_stack(self, content): |
67 | return config.MemoryStack(content.encode('utf-8')) |
68 | @@ -58,6 +55,7 @@ |
69 | self.overrideAttr(_urllib2_wrappers.opt_ssl_ca_certs, 'default', |
70 | os.path.join(self.test_dir, u"nonexisting.pem")) |
71 | self.warnings = [] |
72 | + |
73 | def warning(*args): |
74 | self.warnings.append(args[0] % args[1:]) |
75 | self.overrideAttr(trace, 'warning', warning) |
76 | @@ -67,7 +65,7 @@ |
77 | "is not valid for \"ssl.ca_certs\"") |
78 | |
79 | |
80 | -class CertReqsConfigTests(TestCaseInTempDir): |
81 | +class CertReqsConfigTests(tests.TestCaseInTempDir): |
82 | |
83 | def test_default(self): |
84 | stack = config.MemoryStack("") |
85 | @@ -82,35 +80,41 @@ |
86 | self.assertRaises(ConfigOptionValueError, stack.get, "ssl.cert_reqs") |
87 | |
88 | |
89 | -class MatchHostnameTests(TestCase): |
90 | +class MatchHostnameTests(tests.TestCase): |
91 | + |
92 | + def setUp(self): |
93 | + super(MatchHostnameTests, self).setUp() |
94 | + if sys.version_info < (2, 7, 9): |
95 | + raise tests.TestSkipped( |
96 | + 'python version too old to provide proper' |
97 | + ' https hostname verification') |
98 | |
99 | def test_no_certificate(self): |
100 | self.assertRaises(ValueError, |
101 | - _urllib2_wrappers.match_hostname, {}, "example.com") |
102 | + ssl.match_hostname, {}, "example.com") |
103 | |
104 | def test_wildcards_in_cert(self): |
105 | def ok(cert, hostname): |
106 | - _urllib2_wrappers.match_hostname(cert, hostname) |
107 | + ssl.match_hostname(cert, hostname) |
108 | + |
109 | + def not_ok(cert, hostname): |
110 | + self.assertRaises( |
111 | + ssl.CertificateError, |
112 | + ssl.match_hostname, cert, hostname) |
113 | |
114 | # Python Issue #17980: avoid denials of service by refusing more than |
115 | # one wildcard per fragment. |
116 | - cert = {'subject': ((('commonName', 'a*b.com'),),)} |
117 | - ok(cert, 'axxb.com') |
118 | - cert = {'subject': ((('commonName', 'a*b.co*'),),)} |
119 | - ok(cert, 'axxb.com') |
120 | - cert = {'subject': ((('commonName', 'a*b*.com'),),)} |
121 | - try: |
122 | - _urllib2_wrappers.match_hostname(cert, 'axxbxxc.com') |
123 | - except ValueError as e: |
124 | - self.assertIn("too many wildcards", str(e)) |
125 | + ok({'subject': ((('commonName', 'a*b.com'),),)}, 'axxb.com') |
126 | + not_ok({'subject': ((('commonName', 'a*b.co*'),),)}, 'axxb.com') |
127 | + not_ok({'subject': ((('commonName', 'a*b*.com'),),)}, 'axxbxxc.com') |
128 | |
129 | def test_no_valid_attributes(self): |
130 | - self.assertRaises(CertificateError, _urllib2_wrappers.match_hostname, |
131 | + self.assertRaises(ssl.CertificateError, ssl.match_hostname, |
132 | {"Problem": "Solved"}, "example.com") |
133 | |
134 | def test_common_name(self): |
135 | cert = {'subject': ((('commonName', 'example.com'),),)} |
136 | self.assertIs(None, |
137 | - _urllib2_wrappers.match_hostname(cert, "example.com")) |
138 | - self.assertRaises(CertificateError, _urllib2_wrappers.match_hostname, |
139 | + ssl.match_hostname(cert, "example.com")) |
140 | + self.assertRaises(ssl.CertificateError, ssl.match_hostname, |
141 | cert, "example.org") |
142 | |
143 | === modified file 'bzrlib/transport/http/_urllib2_wrappers.py' |
144 | --- bzrlib/transport/http/_urllib2_wrappers.py 2013-05-20 16:38:11 +0000 |
145 | +++ bzrlib/transport/http/_urllib2_wrappers.py 2016-01-31 13:08:48 +0000 |
146 | @@ -1,4 +1,4 @@ |
147 | -# Copyright (C) 2006-2012 Canonical Ltd |
148 | +# Copyright (C) 2006-2013, 2016 Canonical Ltd |
149 | # |
150 | # This program is free software; you can redistribute it and/or modify |
151 | # it under the terms of the GNU General Public License as published by |
152 | @@ -56,6 +56,7 @@ |
153 | import urllib2 |
154 | import urlparse |
155 | import re |
156 | +import ssl |
157 | import sys |
158 | import time |
159 | |
160 | @@ -70,23 +71,33 @@ |
161 | transport, |
162 | ui, |
163 | urlutils, |
164 | - ) |
165 | -lazy_import.lazy_import(globals(), """ |
166 | -import ssl |
167 | -""") |
168 | +) |
169 | + |
170 | +try: |
171 | + _ = (ssl.match_hostname, ssl.CertificateError) |
172 | +except AttributeError: |
173 | + # Provide fallbacks for python < 2.7.9 |
174 | + def match_hostname(cert, host): |
175 | + trace.warning( |
176 | + '%s cannot be verified, https certificates verification is only' |
177 | + ' available for python versions >= 2.7.9' % (host,)) |
178 | + ssl.match_hostname = match_hostname |
179 | + ssl.CertificateError = ValueError |
180 | |
181 | |
182 | # Note for packagers: if there is no package providing certs for your platform, |
183 | # the curl project produces http://curl.haxx.se/ca/cacert.pem weekly. |
184 | _ssl_ca_certs_known_locations = [ |
185 | - u'/etc/ssl/certs/ca-certificates.crt', # Ubuntu/debian/gentoo |
186 | - u'/etc/pki/tls/certs/ca-bundle.crt', # Fedora/CentOS/RH |
187 | - u'/etc/ssl/ca-bundle.pem', # OpenSuse |
188 | - u'/etc/ssl/cert.pem', # OpenSuse |
189 | - u"/usr/local/share/certs/ca-root-nss.crt", # FreeBSD |
190 | + u'/etc/ssl/certs/ca-certificates.crt', # Ubuntu/debian/gentoo |
191 | + u'/etc/pki/tls/certs/ca-bundle.crt', # Fedora/CentOS/RH |
192 | + u'/etc/ssl/ca-bundle.pem', # OpenSuse |
193 | + u'/etc/ssl/cert.pem', # OpenSuse |
194 | + u"/usr/local/share/certs/ca-root-nss.crt", # FreeBSD |
195 | # XXX: Needs checking, can't trust the interweb ;) -- vila 2012-01-25 |
196 | - u'/etc/openssl/certs/ca-certificates.crt', # Solaris |
197 | - ] |
198 | + u'/etc/openssl/certs/ca-certificates.crt', # Solaris |
199 | +] |
200 | + |
201 | + |
202 | def default_ca_certs(): |
203 | if sys.platform == 'win32': |
204 | return os.path.join(os.path.dirname(sys.executable), u"cacert.pem") |
205 | @@ -115,13 +126,12 @@ |
206 | def cert_reqs_from_store(unicode_str): |
207 | import ssl |
208 | try: |
209 | - return { |
210 | - "required": ssl.CERT_REQUIRED, |
211 | - "none": ssl.CERT_NONE |
212 | - }[unicode_str] |
213 | + return {"required": ssl.CERT_REQUIRED, |
214 | + "none": ssl.CERT_NONE}[unicode_str] |
215 | except KeyError: |
216 | raise ValueError("invalid value %s" % unicode_str) |
217 | |
218 | + |
219 | def default_ca_reqs(): |
220 | if sys.platform in ('win32', 'darwin'): |
221 | # FIXME: Once we get a native access to root certificates there, this |
222 | @@ -131,10 +141,10 @@ |
223 | return u'required' |
224 | |
225 | opt_ssl_ca_certs = config.Option('ssl.ca_certs', |
226 | - from_unicode=ca_certs_from_store, |
227 | - default=default_ca_certs, |
228 | - invalid='warning', |
229 | - help="""\ |
230 | + from_unicode=ca_certs_from_store, |
231 | + default=default_ca_certs, |
232 | + invalid='warning', |
233 | + help="""\ |
234 | Path to certification authority certificates to trust. |
235 | |
236 | This should be a valid path to a bundle containing all root Certificate |
237 | @@ -144,10 +154,10 @@ |
238 | """) |
239 | |
240 | opt_ssl_cert_reqs = config.Option('ssl.cert_reqs', |
241 | - default=default_ca_reqs, |
242 | - from_unicode=cert_reqs_from_store, |
243 | - invalid='error', |
244 | - help="""\ |
245 | + default=default_ca_reqs, |
246 | + from_unicode=cert_reqs_from_store, |
247 | + invalid='error', |
248 | + help="""\ |
249 | Whether to require a certificate from the remote side. (default:required) |
250 | |
251 | Possible values: |
252 | @@ -398,68 +408,6 @@ |
253 | self._wrap_socket_for_reporting(self.sock) |
254 | |
255 | |
256 | -# These two methods were imported from Python 3.2's ssl module |
257 | - |
258 | -def _dnsname_to_pat(dn, max_wildcards=1): |
259 | - pats = [] |
260 | - for frag in dn.split(r'.'): |
261 | - if frag.count('*') > max_wildcards: |
262 | - # Python Issue #17980: avoid denials of service by refusing more |
263 | - # than one wildcard per fragment. A survery of established |
264 | - # policy among SSL implementations showed it to be a |
265 | - # reasonable choice. |
266 | - raise ValueError( |
267 | - "too many wildcards in certificate DNS name: " + repr(dn)) |
268 | - if frag == '*': |
269 | - # When '*' is a fragment by itself, it matches a non-empty dotless |
270 | - # fragment. |
271 | - pats.append('[^.]+') |
272 | - else: |
273 | - # Otherwise, '*' matches any dotless fragment. |
274 | - frag = re.escape(frag) |
275 | - pats.append(frag.replace(r'\*', '[^.]*')) |
276 | - return re.compile(r'\A' + r'\.'.join(pats) + r'\Z', re.IGNORECASE) |
277 | - |
278 | - |
279 | -def match_hostname(cert, hostname): |
280 | - """Verify that *cert* (in decoded format as returned by |
281 | - SSLSocket.getpeercert()) matches the *hostname*. RFC 2818 rules |
282 | - are mostly followed, but IP addresses are not accepted for *hostname*. |
283 | - |
284 | - CertificateError is raised on failure. On success, the function |
285 | - returns nothing. |
286 | - """ |
287 | - if not cert: |
288 | - raise ValueError("empty or no certificate") |
289 | - dnsnames = [] |
290 | - san = cert.get('subjectAltName', ()) |
291 | - for key, value in san: |
292 | - if key == 'DNS': |
293 | - if _dnsname_to_pat(value).match(hostname): |
294 | - return |
295 | - dnsnames.append(value) |
296 | - if not san: |
297 | - # The subject is only checked when subjectAltName is empty |
298 | - for sub in cert.get('subject', ()): |
299 | - for key, value in sub: |
300 | - # XXX according to RFC 2818, the most specific Common Name |
301 | - # must be used. |
302 | - if key == 'commonName': |
303 | - if _dnsname_to_pat(value).match(hostname): |
304 | - return |
305 | - dnsnames.append(value) |
306 | - if len(dnsnames) > 1: |
307 | - raise errors.CertificateError( |
308 | - "hostname %r doesn't match either of %s" |
309 | - % (hostname, ', '.join(map(repr, dnsnames)))) |
310 | - elif len(dnsnames) == 1: |
311 | - raise errors.CertificateError("hostname %r doesn't match %r" % |
312 | - (hostname, dnsnames[0])) |
313 | - else: |
314 | - raise errors.CertificateError("no appropriate commonName or " |
315 | - "subjectAltName fields were found") |
316 | - |
317 | - |
318 | class HTTPSConnection(AbstractHTTPConnection, httplib.HTTPSConnection): |
319 | |
320 | def __init__(self, host, port=None, key_file=None, cert_file=None, |
321 | @@ -503,9 +451,10 @@ |
322 | "'bzr help ssl.ca_certs' for more information on setting " |
323 | "trusted CAs.") |
324 | try: |
325 | - ssl_sock = ssl.wrap_socket(self.sock, self.key_file, self.cert_file, |
326 | + ssl_sock = ssl.wrap_socket( |
327 | + self.sock, self.key_file, self.cert_file, |
328 | cert_reqs=cert_reqs, ca_certs=ca_certs) |
329 | - except ssl.SSLError, e: |
330 | + except ssl.SSLError: |
331 | trace.note( |
332 | "\n" |
333 | "See `bzr help ssl.ca_certs` for how to specify trusted CA" |
334 | @@ -515,7 +464,7 @@ |
335 | raise |
336 | if cert_reqs == ssl.CERT_REQUIRED: |
337 | peer_cert = ssl_sock.getpeercert() |
338 | - match_hostname(peer_cert, host) |
339 | + ssl.match_hostname(peer_cert, host) |
340 | |
341 | # Wrap the ssl socket before anybody use it |
342 | self._wrap_socket_for_reporting(ssl_sock) |
343 | @@ -833,7 +782,7 @@ |
344 | % (request, request.connection.sock.getsockname()) |
345 | response = connection.getresponse() |
346 | convert_to_addinfourl = True |
347 | - except (ssl.SSLError, errors.CertificateError): |
348 | + except (ssl.SSLError, ssl.CertificateError): |
349 | # Something is wrong with either the certificate or the hostname, |
350 | # re-trying won't help |
351 | raise |
352 | |
353 | === modified file 'doc/en/release-notes/bzr-2.7.txt' |
354 | --- doc/en/release-notes/bzr-2.7.txt 2016-01-22 08:02:12 +0000 |
355 | +++ doc/en/release-notes/bzr-2.7.txt 2016-01-31 13:08:48 +0000 |
356 | @@ -70,6 +70,9 @@ |
357 | or UnicodeEncodeError when given unicode strings rather than bytes. |
358 | (Vincent Ladeuil, #106898) |
359 | |
360 | +* Use ssl.match_hostname from the python ssl module and stop carrying a |
361 | + specific version that has become obsolete. (Vincent Ladeuil, #1538480) |
362 | + |
363 | Changed Behaviour |
364 | ***************** |
365 |
Vincent, what you've done here is excellent and definitely an improvement over the contributed patch, exempli gratia, adding the not_ok function.
What I'm curious about is this patch requires python >= 2.7.9. Do we plan to bump our python version requirement from 2.6 to 2.7.9?
The other part of the patch checks for match_hostname in the ssl library and, if not found, imports it from backports.
1. Is that a worthwhile exercise in light of python version requirements?
2. Is backports normally available or would we change our dependencies to get it?