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

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
=== modified file 'bzrlib/errors.py'
--- bzrlib/errors.py 2013-10-04 09:56:23 +0000
+++ bzrlib/errors.py 2016-01-31 13:08:48 +0000
@@ -1,4 +1,4 @@
1# Copyright (C) 2005-2011 Canonical Ltd1# Copyright (C) 2005-2013, 2016 Canonical Ltd
2#2#
3# This program is free software; you can redistribute it and/or modify3# This program is free software; you can redistribute it and/or modify
4# it under the terms of the GNU General Public License as published by4# it under the terms of the GNU General Public License as published by
@@ -1686,14 +1686,6 @@
1686 TransportError.__init__(self, msg, orig_error=orig_error)1686 TransportError.__init__(self, msg, orig_error=orig_error)
16871687
16881688
1689class CertificateError(TransportError):
1690
1691 _fmt = "Certificate error: %(error)s"
1692
1693 def __init__(self, error):
1694 self.error = error
1695
1696
1697class InvalidHttpRange(InvalidHttpResponse):1689class InvalidHttpRange(InvalidHttpResponse):
16981690
1699 _fmt = "Invalid http range %(range)r for %(path)s: %(msg)s"1691 _fmt = "Invalid http range %(range)r for %(path)s: %(msg)s"
17001692
=== modified file 'bzrlib/tests/test_https_urllib.py'
--- bzrlib/tests/test_https_urllib.py 2013-05-20 16:38:11 +0000
+++ bzrlib/tests/test_https_urllib.py 2016-01-31 13:08:48 +0000
@@ -1,4 +1,4 @@
1# Copyright (C) 2011,2012 Canonical Ltd1# Copyright (C) 2011, 2012, 2013, 2016 Canonical Ltd
2#2#
3# This program is free software; you can redistribute it and/or modify3# This program is free software; you can redistribute it and/or modify
4# it under the terms of the GNU General Public License as published by4# it under the terms of the GNU General Public License as published by
@@ -19,24 +19,21 @@
19"""19"""
2020
21import os21import os
22import ssl22import sys
2323
24from bzrlib import (24from bzrlib import (
25 config,25 config,
26 trace,26 trace,
27 )27)
28from bzrlib.errors import (28from bzrlib.errors import (
29 CertificateError,
30 ConfigOptionValueError,29 ConfigOptionValueError,
31 )30)
32from bzrlib.tests import (31from bzrlib import tests
33 TestCase,
34 TestCaseInTempDir,
35 )
36from bzrlib.transport.http import _urllib2_wrappers32from bzrlib.transport.http import _urllib2_wrappers
3733from bzrlib.transport.http._urllib2_wrappers import ssl
3834
39class CaCertsConfigTests(TestCaseInTempDir):35
36class CaCertsConfigTests(tests.TestCaseInTempDir):
4037
41 def get_stack(self, content):38 def get_stack(self, content):
42 return config.MemoryStack(content.encode('utf-8'))39 return config.MemoryStack(content.encode('utf-8'))
@@ -58,6 +55,7 @@
58 self.overrideAttr(_urllib2_wrappers.opt_ssl_ca_certs, 'default',55 self.overrideAttr(_urllib2_wrappers.opt_ssl_ca_certs, 'default',
59 os.path.join(self.test_dir, u"nonexisting.pem"))56 os.path.join(self.test_dir, u"nonexisting.pem"))
60 self.warnings = []57 self.warnings = []
58
61 def warning(*args):59 def warning(*args):
62 self.warnings.append(args[0] % args[1:])60 self.warnings.append(args[0] % args[1:])
63 self.overrideAttr(trace, 'warning', warning)61 self.overrideAttr(trace, 'warning', warning)
@@ -67,7 +65,7 @@
67 "is not valid for \"ssl.ca_certs\"")65 "is not valid for \"ssl.ca_certs\"")
6866
6967
70class CertReqsConfigTests(TestCaseInTempDir):68class CertReqsConfigTests(tests.TestCaseInTempDir):
7169
72 def test_default(self):70 def test_default(self):
73 stack = config.MemoryStack("")71 stack = config.MemoryStack("")
@@ -82,35 +80,41 @@
82 self.assertRaises(ConfigOptionValueError, stack.get, "ssl.cert_reqs")80 self.assertRaises(ConfigOptionValueError, stack.get, "ssl.cert_reqs")
8381
8482
85class MatchHostnameTests(TestCase):83class MatchHostnameTests(tests.TestCase):
84
85 def setUp(self):
86 super(MatchHostnameTests, self).setUp()
87 if sys.version_info < (2, 7, 9):
88 raise tests.TestSkipped(
89 'python version too old to provide proper'
90 ' https hostname verification')
8691
87 def test_no_certificate(self):92 def test_no_certificate(self):
88 self.assertRaises(ValueError,93 self.assertRaises(ValueError,
89 _urllib2_wrappers.match_hostname, {}, "example.com")94 ssl.match_hostname, {}, "example.com")
9095
91 def test_wildcards_in_cert(self):96 def test_wildcards_in_cert(self):
92 def ok(cert, hostname):97 def ok(cert, hostname):
93 _urllib2_wrappers.match_hostname(cert, hostname)98 ssl.match_hostname(cert, hostname)
99
100 def not_ok(cert, hostname):
101 self.assertRaises(
102 ssl.CertificateError,
103 ssl.match_hostname, cert, hostname)
94104
95 # Python Issue #17980: avoid denials of service by refusing more than105 # Python Issue #17980: avoid denials of service by refusing more than
96 # one wildcard per fragment.106 # one wildcard per fragment.
97 cert = {'subject': ((('commonName', 'a*b.com'),),)}107 ok({'subject': ((('commonName', 'a*b.com'),),)}, 'axxb.com')
98 ok(cert, 'axxb.com')108 not_ok({'subject': ((('commonName', 'a*b.co*'),),)}, 'axxb.com')
99 cert = {'subject': ((('commonName', 'a*b.co*'),),)}109 not_ok({'subject': ((('commonName', 'a*b*.com'),),)}, 'axxbxxc.com')
100 ok(cert, 'axxb.com')
101 cert = {'subject': ((('commonName', 'a*b*.com'),),)}
102 try:
103 _urllib2_wrappers.match_hostname(cert, 'axxbxxc.com')
104 except ValueError as e:
105 self.assertIn("too many wildcards", str(e))
106110
107 def test_no_valid_attributes(self):111 def test_no_valid_attributes(self):
108 self.assertRaises(CertificateError, _urllib2_wrappers.match_hostname,112 self.assertRaises(ssl.CertificateError, ssl.match_hostname,
109 {"Problem": "Solved"}, "example.com")113 {"Problem": "Solved"}, "example.com")
110114
111 def test_common_name(self):115 def test_common_name(self):
112 cert = {'subject': ((('commonName', 'example.com'),),)}116 cert = {'subject': ((('commonName', 'example.com'),),)}
113 self.assertIs(None,117 self.assertIs(None,
114 _urllib2_wrappers.match_hostname(cert, "example.com"))118 ssl.match_hostname(cert, "example.com"))
115 self.assertRaises(CertificateError, _urllib2_wrappers.match_hostname,119 self.assertRaises(ssl.CertificateError, ssl.match_hostname,
116 cert, "example.org")120 cert, "example.org")
117121
=== modified file 'bzrlib/transport/http/_urllib2_wrappers.py'
--- bzrlib/transport/http/_urllib2_wrappers.py 2013-05-20 16:38:11 +0000
+++ bzrlib/transport/http/_urllib2_wrappers.py 2016-01-31 13:08:48 +0000
@@ -1,4 +1,4 @@
1# Copyright (C) 2006-2012 Canonical Ltd1# Copyright (C) 2006-2013, 2016 Canonical Ltd
2#2#
3# This program is free software; you can redistribute it and/or modify3# This program is free software; you can redistribute it and/or modify
4# it under the terms of the GNU General Public License as published by4# it under the terms of the GNU General Public License as published by
@@ -56,6 +56,7 @@
56import urllib256import urllib2
57import urlparse57import urlparse
58import re58import re
59import ssl
59import sys60import sys
60import time61import time
6162
@@ -70,23 +71,33 @@
70 transport,71 transport,
71 ui,72 ui,
72 urlutils,73 urlutils,
73 )74)
74lazy_import.lazy_import(globals(), """75
75import ssl76try:
76""")77 _ = (ssl.match_hostname, ssl.CertificateError)
78except AttributeError:
79 # Provide fallbacks for python < 2.7.9
80 def match_hostname(cert, host):
81 trace.warning(
82 '%s cannot be verified, https certificates verification is only'
83 ' available for python versions >= 2.7.9' % (host,))
84 ssl.match_hostname = match_hostname
85 ssl.CertificateError = ValueError
7786
7887
79# Note for packagers: if there is no package providing certs for your platform,88# Note for packagers: if there is no package providing certs for your platform,
80# the curl project produces http://curl.haxx.se/ca/cacert.pem weekly.89# the curl project produces http://curl.haxx.se/ca/cacert.pem weekly.
81_ssl_ca_certs_known_locations = [90_ssl_ca_certs_known_locations = [
82 u'/etc/ssl/certs/ca-certificates.crt', # Ubuntu/debian/gentoo91 u'/etc/ssl/certs/ca-certificates.crt', # Ubuntu/debian/gentoo
83 u'/etc/pki/tls/certs/ca-bundle.crt', # Fedora/CentOS/RH92 u'/etc/pki/tls/certs/ca-bundle.crt', # Fedora/CentOS/RH
84 u'/etc/ssl/ca-bundle.pem', # OpenSuse93 u'/etc/ssl/ca-bundle.pem', # OpenSuse
85 u'/etc/ssl/cert.pem', # OpenSuse94 u'/etc/ssl/cert.pem', # OpenSuse
86 u"/usr/local/share/certs/ca-root-nss.crt", # FreeBSD95 u"/usr/local/share/certs/ca-root-nss.crt", # FreeBSD
87 # XXX: Needs checking, can't trust the interweb ;) -- vila 2012-01-2596 # XXX: Needs checking, can't trust the interweb ;) -- vila 2012-01-25
88 u'/etc/openssl/certs/ca-certificates.crt', # Solaris97 u'/etc/openssl/certs/ca-certificates.crt', # Solaris
89 ]98]
99
100
90def default_ca_certs():101def default_ca_certs():
91 if sys.platform == 'win32':102 if sys.platform == 'win32':
92 return os.path.join(os.path.dirname(sys.executable), u"cacert.pem")103 return os.path.join(os.path.dirname(sys.executable), u"cacert.pem")
@@ -115,13 +126,12 @@
115def cert_reqs_from_store(unicode_str):126def cert_reqs_from_store(unicode_str):
116 import ssl127 import ssl
117 try:128 try:
118 return {129 return {"required": ssl.CERT_REQUIRED,
119 "required": ssl.CERT_REQUIRED,130 "none": ssl.CERT_NONE}[unicode_str]
120 "none": ssl.CERT_NONE
121 }[unicode_str]
122 except KeyError:131 except KeyError:
123 raise ValueError("invalid value %s" % unicode_str)132 raise ValueError("invalid value %s" % unicode_str)
124133
134
125def default_ca_reqs():135def default_ca_reqs():
126 if sys.platform in ('win32', 'darwin'):136 if sys.platform in ('win32', 'darwin'):
127 # FIXME: Once we get a native access to root certificates there, this137 # FIXME: Once we get a native access to root certificates there, this
@@ -131,10 +141,10 @@
131 return u'required'141 return u'required'
132142
133opt_ssl_ca_certs = config.Option('ssl.ca_certs',143opt_ssl_ca_certs = config.Option('ssl.ca_certs',
134 from_unicode=ca_certs_from_store,144 from_unicode=ca_certs_from_store,
135 default=default_ca_certs,145 default=default_ca_certs,
136 invalid='warning',146 invalid='warning',
137 help="""\147 help="""\
138Path to certification authority certificates to trust.148Path to certification authority certificates to trust.
139149
140This should be a valid path to a bundle containing all root Certificate150This should be a valid path to a bundle containing all root Certificate
@@ -144,10 +154,10 @@
144""")154""")
145155
146opt_ssl_cert_reqs = config.Option('ssl.cert_reqs',156opt_ssl_cert_reqs = config.Option('ssl.cert_reqs',
147 default=default_ca_reqs,157 default=default_ca_reqs,
148 from_unicode=cert_reqs_from_store,158 from_unicode=cert_reqs_from_store,
149 invalid='error',159 invalid='error',
150 help="""\160 help="""\
151Whether to require a certificate from the remote side. (default:required)161Whether to require a certificate from the remote side. (default:required)
152162
153Possible values:163Possible values:
@@ -398,68 +408,6 @@
398 self._wrap_socket_for_reporting(self.sock)408 self._wrap_socket_for_reporting(self.sock)
399409
400410
401# These two methods were imported from Python 3.2's ssl module
402
403def _dnsname_to_pat(dn, max_wildcards=1):
404 pats = []
405 for frag in dn.split(r'.'):
406 if frag.count('*') > max_wildcards:
407 # Python Issue #17980: avoid denials of service by refusing more
408 # than one wildcard per fragment. A survery of established
409 # policy among SSL implementations showed it to be a
410 # reasonable choice.
411 raise ValueError(
412 "too many wildcards in certificate DNS name: " + repr(dn))
413 if frag == '*':
414 # When '*' is a fragment by itself, it matches a non-empty dotless
415 # fragment.
416 pats.append('[^.]+')
417 else:
418 # Otherwise, '*' matches any dotless fragment.
419 frag = re.escape(frag)
420 pats.append(frag.replace(r'\*', '[^.]*'))
421 return re.compile(r'\A' + r'\.'.join(pats) + r'\Z', re.IGNORECASE)
422
423
424def match_hostname(cert, hostname):
425 """Verify that *cert* (in decoded format as returned by
426 SSLSocket.getpeercert()) matches the *hostname*. RFC 2818 rules
427 are mostly followed, but IP addresses are not accepted for *hostname*.
428
429 CertificateError is raised on failure. On success, the function
430 returns nothing.
431 """
432 if not cert:
433 raise ValueError("empty or no certificate")
434 dnsnames = []
435 san = cert.get('subjectAltName', ())
436 for key, value in san:
437 if key == 'DNS':
438 if _dnsname_to_pat(value).match(hostname):
439 return
440 dnsnames.append(value)
441 if not san:
442 # The subject is only checked when subjectAltName is empty
443 for sub in cert.get('subject', ()):
444 for key, value in sub:
445 # XXX according to RFC 2818, the most specific Common Name
446 # must be used.
447 if key == 'commonName':
448 if _dnsname_to_pat(value).match(hostname):
449 return
450 dnsnames.append(value)
451 if len(dnsnames) > 1:
452 raise errors.CertificateError(
453 "hostname %r doesn't match either of %s"
454 % (hostname, ', '.join(map(repr, dnsnames))))
455 elif len(dnsnames) == 1:
456 raise errors.CertificateError("hostname %r doesn't match %r" %
457 (hostname, dnsnames[0]))
458 else:
459 raise errors.CertificateError("no appropriate commonName or "
460 "subjectAltName fields were found")
461
462
463class HTTPSConnection(AbstractHTTPConnection, httplib.HTTPSConnection):411class HTTPSConnection(AbstractHTTPConnection, httplib.HTTPSConnection):
464412
465 def __init__(self, host, port=None, key_file=None, cert_file=None,413 def __init__(self, host, port=None, key_file=None, cert_file=None,
@@ -503,9 +451,10 @@
503 "'bzr help ssl.ca_certs' for more information on setting "451 "'bzr help ssl.ca_certs' for more information on setting "
504 "trusted CAs.")452 "trusted CAs.")
505 try:453 try:
506 ssl_sock = ssl.wrap_socket(self.sock, self.key_file, self.cert_file,454 ssl_sock = ssl.wrap_socket(
455 self.sock, self.key_file, self.cert_file,
507 cert_reqs=cert_reqs, ca_certs=ca_certs)456 cert_reqs=cert_reqs, ca_certs=ca_certs)
508 except ssl.SSLError, e:457 except ssl.SSLError:
509 trace.note(458 trace.note(
510 "\n"459 "\n"
511 "See `bzr help ssl.ca_certs` for how to specify trusted CA"460 "See `bzr help ssl.ca_certs` for how to specify trusted CA"
@@ -515,7 +464,7 @@
515 raise464 raise
516 if cert_reqs == ssl.CERT_REQUIRED:465 if cert_reqs == ssl.CERT_REQUIRED:
517 peer_cert = ssl_sock.getpeercert()466 peer_cert = ssl_sock.getpeercert()
518 match_hostname(peer_cert, host)467 ssl.match_hostname(peer_cert, host)
519468
520 # Wrap the ssl socket before anybody use it469 # Wrap the ssl socket before anybody use it
521 self._wrap_socket_for_reporting(ssl_sock)470 self._wrap_socket_for_reporting(ssl_sock)
@@ -833,7 +782,7 @@
833 % (request, request.connection.sock.getsockname())782 % (request, request.connection.sock.getsockname())
834 response = connection.getresponse()783 response = connection.getresponse()
835 convert_to_addinfourl = True784 convert_to_addinfourl = True
836 except (ssl.SSLError, errors.CertificateError):785 except (ssl.SSLError, ssl.CertificateError):
837 # Something is wrong with either the certificate or the hostname,786 # Something is wrong with either the certificate or the hostname,
838 # re-trying won't help787 # re-trying won't help
839 raise788 raise
840789
=== modified file 'doc/en/release-notes/bzr-2.7.txt'
--- doc/en/release-notes/bzr-2.7.txt 2016-01-22 08:02:12 +0000
+++ doc/en/release-notes/bzr-2.7.txt 2016-01-31 13:08:48 +0000
@@ -70,6 +70,9 @@
70 or UnicodeEncodeError when given unicode strings rather than bytes.70 or UnicodeEncodeError when given unicode strings rather than bytes.
71 (Vincent Ladeuil, #106898)71 (Vincent Ladeuil, #106898)
7272
73* Use ssl.match_hostname from the python ssl module and stop carrying a
74 specific version that has become obsolete. (Vincent Ladeuil, #1538480)
75
73Changed Behaviour76Changed Behaviour
74*****************77*****************
7578