Bazaar

Merge lp:~vila/bzr/1538480-match-hostname into lp:bzr

  1. 1538480-match-hostname
  2. Merge into bzr.dev
Proposed by Vincent Ladeuil
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
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.

To post a comment you must log in.
Revision history for this message
Richard Wilbur (richard-wilbur) 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?

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?

Revision history for this message
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://docs.python.org/2.6/ and saw 'last updated Oct 29, 2013'. I.e. python 2.6 didn't receive any security support for the last 2 years.

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.

Revision history for this message
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?

review: Needs Fixing
Revision history for this message
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.

Revision history for this message
Richard Wilbur (richard-wilbur) wrote :

Thanks for that last revision. I think a descriptive warning is significantly better than an exception!
+2

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

sent to pqm by email

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