openstack/python-glanceclient
Code Issues Proposed changes

Remove custom SSL compression handling

Browse Source 
Custom SSL handling was introduced because disabling SSL layer compression
provided an approximately five fold performance increase in some
cases. Without SSL layer compression disabled the image transfer would be
CPU bound -- with the CPU performing the DEFLATE algorithm.  This would
typically limit image transfers to < 20 MB/s. When --no-ssl-compression
was specified the client would not negotiate any compression algorithm
during the SSL handshake with the server which would remove the CPU
bottleneck and transfers could approach wire speed.

In order to support '--no-ssl-compression' two totally separate code
paths exist depending on whether this is True or False.  When SSL
compression is disabled, rather than using the standard 'requests'
library, we enter some custom code based on pyopenssl and httplib in
order to disable compression.

This patch/spec proposes removing the custom code because:

* It is a burden to maintain

 Eg adding new code such as keystone session support is more complicated

* It can introduce additional failure modes

 We have seen some bugs related to the 'custom' certificate checking

* Newer Operating Systems disable SSL for us.

 Eg. While Debian 7 defaulted to compression 'on', Debian 8 has compression
 'off'. This makes both servers and client less likely to have compression
 enabled.

* Newer combinations of 'requests' and 'python' do this for us

 Requests disables compression when backed by a version of python which
 supports it (>= 2.7.9). This makes clients more likely to disable
 compression out-of-the-box.

* It is (in principle) possible to do this on older versions too

 If pyopenssl, ndg-httpsclient and pyasn1 are installed on older
 operating system/python combinations, the requests library should
 disable SSL compression on the client side.

* Systems that have SSL compression enabled may be vulnerable to the CRIME
(https://web.nvd.nist.gov/view/vuln/detail?vulnId=CVE-2012-4929) attack.
Installations which are security conscious should be running the Glance
server with SSL disabled.

Full Spec: https://review.openstack.org/#/c/187674
Blueprint: remove-custom-client-ssl-handling

Change-Id: I7e7761fc91b0d6da03939374eeedd809534f6edf
This commit is contained in:
Stuart McLaren
2015-06-08 14:49:52 +00:00
parent 17375a422c
commit 618637a5bd
7 changed files with 129 additions and 335 deletions
Download Patch File Download Diff File Expand all files Collapse all files

23
glanceclient/common/http.py
View File

@@ -28,6 +28,7 @@ except ImportError:
ProtocolError = requests.exceptions.ConnectionError
import six
from six.moves.urllib import parse
import warnings
try:
import json
@@ -41,7 +42,6 @@ if not hasattr(parse, 'parse_qsl'):
from oslo_utils import encodeutils
from glanceclient.common import https
from glanceclient.common import utils
from glanceclient import exc
@@ -138,20 +138,17 @@ class HTTPClient(_BaseHTTPClient):
if self.endpoint.startswith("https"):
compression = kwargs.get('ssl_compression', True)
if not compression:
self.session.mount("glance+https://", https.HTTPSAdapter())
self.endpoint = 'glance+' + self.endpoint
self.session.verify = (
kwargs.get('cacert', requests.certs.where()),
kwargs.get('insecure', False))
if compression is False:
# Note: This is not seen by default. (python must be
# run with -Wd)
warnings.warn('The "ssl_compression" argument has been '
'deprecated.', DeprecationWarning)
if kwargs.get('insecure', False) is True:
self.session.verify = False
else:
if kwargs.get('insecure', False) is True:
self.session.verify = False
else:
if kwargs.get('cacert', None) is not '':
self.session.verify = kwargs.get('cacert', True)
if kwargs.get('cacert', None) is not '':
self.session.verify = kwargs.get('cacert', True)
self.session.cert = (kwargs.get('cert_file'),
kwargs.get('key_file'))

12
glanceclient/common/utils.py
View File

@@ -318,18 +318,6 @@ def make_size_human_readable(size):
return '%s%s' % (stripped, suffix[index])
def getsockopt(self, *args, **kwargs):
"""
A function which allows us to monkey patch eventlet's
GreenSocket, adding a required 'getsockopt' method.
TODO: (mclaren) we can remove this once the eventlet fix
(https://bitbucket.org/eventlet/eventlet/commits/609f230)
lands in mainstream packages.
"""
return self.fd.getsockopt(*args, **kwargs)
def exception_to_str(exc):
try:
error = six.text_type(exc)

5
glanceclient/shell.py
View File

@@ -241,7 +241,10 @@ class OpenStackImagesShell(object):
parser.add_argument('--no-ssl-compression',
dest='ssl_compression',
default=True, action='store_false',
help='Disable SSL compression when using https.')
help='DEPRECATED! This option is deprecated '
'and not used anymore. SSL compression '
'should be disabled by default by the '
'system SSL library.')
parser.add_argument('-f', '--force',
dest='force',

9
glanceclient/tests/functional/test_readonly_glance.py
View File

@@ -104,3 +104,12 @@ class SimpleReadOnlyGlanceClientTest(base.ClientTestBase):
def test_debug_list(self):
self.glance('image-list', flags='--debug')
def test_no_ssl_compression(self):
# Test deprecating this hasn't broken anything
out = self.glance('--os-image-api-version 1 '
'--no-ssl-compression image-list')
endpoints = self.parser.listing(out)
self.assertTableStruct(endpoints, [
'ID', 'Name', 'Disk Format', 'Container Format',
'Size', 'Status'])

19
glanceclient/tests/unit/test_http.py
View File

@@ -29,8 +29,6 @@ import types
import glanceclient
from glanceclient.common import http
from glanceclient.common import https
from glanceclient import exc
from glanceclient.tests import utils
@@ -354,20 +352,3 @@ class TestClient(testtools.TestCase):
self.assertThat(mock_log.call_args[0][0],
matchers.Not(matchers.MatchesRegex(token_regex)),
'token found in LOG.debug parameter')
class TestVerifiedHTTPSConnection(testtools.TestCase):
"""Test fixture for glanceclient.common.http.VerifiedHTTPSConnection."""
def test_setcontext_unable_to_load_cacert(self):
"""Add this UT case with Bug#1265730."""
self.assertRaises(exc.SSLConfigurationError,
https.VerifiedHTTPSConnection,
"127.0.0.1",
None,
None,
None,
"gx_cacert",
None,
False,
True)

395
glanceclient/tests/unit/test_ssl.py
View File

@@ -15,20 +15,13 @@
import os
from OpenSSL import crypto
from OpenSSL import SSL
try:
from requests.packages.urllib3 import poolmanager
except ImportError:
from urllib3 import poolmanager
import mock
import six
import ssl
import testtools
import threading
from glanceclient.common import http
from glanceclient.common import https
from glanceclient import Client
from glanceclient import exc
from glanceclient import v1
from glanceclient import v2
@@ -85,7 +78,8 @@ class TestHTTPSVerifyCert(testtools.TestCase):
server_thread.daemon = True
server_thread.start()
def test_v1_requests_cert_verification(self):
@mock.patch('sys.stderr')
def test_v1_requests_cert_verification(self, __):
"""v1 regression test for bug 115260."""
port = self.port
url = 'https://0.0.0.0:%d' % port
@@ -102,8 +96,10 @@ class TestHTTPSVerifyCert(testtools.TestCase):
except Exception:
self.fail('Unexpected exception has been raised')
def test_v1_requests_cert_verification_no_compression(self):
@mock.patch('sys.stderr')
def test_v1_requests_cert_verification_no_compression(self, __):
"""v1 regression test for bug 115260."""
# Legacy test. Verify 'no compression' has no effect
port = self.port
url = 'https://0.0.0.0:%d' % port
@@ -113,13 +109,14 @@ class TestHTTPSVerifyCert(testtools.TestCase):
ssl_compression=False)
client.images.get('image123')
self.fail('No SSL exception has been raised')
except SSL.Error as e:
if 'certificate verify failed' not in str(e):
except exc.CommunicationError as e:
if 'certificate verify failed' not in e.message:
self.fail('No certificate failure message is received')
except Exception:
except Exception as e:
self.fail('Unexpected exception has been raised')
def test_v2_requests_cert_verification(self):
@mock.patch('sys.stderr')
def test_v2_requests_cert_verification(self, __):
"""v2 regression test for bug 115260."""
port = self.port
url = 'https://0.0.0.0:%d' % port
@@ -136,8 +133,10 @@ class TestHTTPSVerifyCert(testtools.TestCase):
except Exception:
self.fail('Unexpected exception has been raised')
def test_v2_requests_cert_verification_no_compression(self):
@mock.patch('sys.stderr')
def test_v2_requests_cert_verification_no_compression(self, __):
"""v2 regression test for bug 115260."""
# Legacy test. Verify 'no compression' has no effect
port = self.port
url = 'https://0.0.0.0:%d' % port
@@ -147,292 +146,110 @@ class TestHTTPSVerifyCert(testtools.TestCase):
ssl_compression=False)
gc.images.get('image123')
self.fail('No SSL exception has been raised')
except SSL.Error as e:
if 'certificate verify failed' not in str(e):
except exc.CommunicationError as e:
if 'certificate verify failed' not in e.message:
self.fail('No certificate failure message is received')
except Exception:
except Exception as e:
self.fail('Unexpected exception has been raised')
class TestVerifiedHTTPSConnection(testtools.TestCase):
def test_ssl_init_ok(self):
"""Test VerifiedHTTPSConnection class init."""
key_file = os.path.join(TEST_VAR_DIR, 'privatekey.key')
cert_file = os.path.join(TEST_VAR_DIR, 'certificate.crt')
@mock.patch('sys.stderr')
def test_v2_requests_valid_cert_verification(self, __):
"""Test absence of SSL key file."""
port = self.port
url = 'https://0.0.0.0:%d' % port
cacert = os.path.join(TEST_VAR_DIR, 'ca.crt')
try:
https.VerifiedHTTPSConnection('127.0.0.1', 0,
key_file=key_file,
cert_file=cert_file,
cacert=cacert)
except exc.SSLConfigurationError:
self.fail('Failed to init VerifiedHTTPSConnection.')
def test_ssl_init_cert_no_key(self):
try:
gc = Client('2', url,
insecure=False,
ssl_compression=True,
cacert=cacert)
gc.images.get('image123')
except exc.CommunicationError as e:
if 'certificate verify failed' in e.message:
self.fail('Certificate failure message is received')
except Exception as e:
self.fail('Unexpected exception has been raised')
@mock.patch('sys.stderr')
def test_v2_requests_valid_cert_verification_no_compression(self, __):
"""Test VerifiedHTTPSConnection: absence of SSL key file."""
port = self.port
url = 'https://0.0.0.0:%d' % port
cacert = os.path.join(TEST_VAR_DIR, 'ca.crt')
try:
gc = Client('2', url,
insecure=False,
ssl_compression=False,
cacert=cacert)
gc.images.get('image123')
except exc.CommunicationError as e:
if 'certificate verify failed' in e.message:
self.fail('Certificate failure message is received')
except Exception as e:
self.fail('Unexpected exception has been raised')
@mock.patch('sys.stderr')
def test_v2_requests_valid_cert_no_key(self, __):
"""Test VerifiedHTTPSConnection: absence of SSL key file."""
port = self.port
url = 'https://0.0.0.0:%d' % port
cert_file = os.path.join(TEST_VAR_DIR, 'certificate.crt')
cacert = os.path.join(TEST_VAR_DIR, 'ca.crt')
try:
https.VerifiedHTTPSConnection('127.0.0.1', 0,
cert_file=cert_file,
cacert=cacert)
self.fail('Failed to raise assertion.')
except exc.SSLConfigurationError:
pass
def test_ssl_init_key_no_cert(self):
"""Test VerifiedHTTPSConnection: absence of SSL cert file."""
key_file = os.path.join(TEST_VAR_DIR, 'privatekey.key')
cacert = os.path.join(TEST_VAR_DIR, 'ca.crt')
try:
https.VerifiedHTTPSConnection('127.0.0.1', 0,
key_file=key_file,
cacert=cacert)
except exc.SSLConfigurationError:
pass
except Exception:
self.fail('Failed to init VerifiedHTTPSConnection.')
gc = Client('2', url,
insecure=False,
ssl_compression=False,
cert_file=cert_file,
cacert=cacert)
gc.images.get('image123')
except exc.CommunicationError as e:
if (six.PY2 and 'PrivateKey' not in e.message or
six.PY3 and 'PEM lib' not in e.message):
self.fail('No appropriate failure message is received')
except Exception as e:
self.fail('Unexpected exception has been raised')
def test_ssl_init_bad_key(self):
"""Test VerifiedHTTPSConnection: bad key."""
cert_file = os.path.join(TEST_VAR_DIR, 'certificate.crt')
cacert = os.path.join(TEST_VAR_DIR, 'ca.crt')
key_file = os.path.join(TEST_VAR_DIR, 'badkey.key')
try:
https.VerifiedHTTPSConnection('127.0.0.1', 0,
key_file=key_file,
cert_file=cert_file,
cacert=cacert)
self.fail('Failed to raise assertion.')
except exc.SSLConfigurationError:
pass
def test_ssl_init_bad_cert(self):
"""Test VerifiedHTTPSConnection: bad cert."""
@mock.patch('sys.stderr')
def test_v2_requests_bad_cert(self, __):
"""Test VerifiedHTTPSConnection: absence of SSL key file."""
port = self.port
url = 'https://0.0.0.0:%d' % port
cert_file = os.path.join(TEST_VAR_DIR, 'badcert.crt')
cacert = os.path.join(TEST_VAR_DIR, 'ca.crt')
try:
https.VerifiedHTTPSConnection('127.0.0.1', 0,
cert_file=cert_file,
cacert=cacert)
self.fail('Failed to raise assertion.')
except exc.SSLConfigurationError:
pass
def test_ssl_init_bad_ca(self):
"""Test VerifiedHTTPSConnection: bad CA."""
cert_file = os.path.join(TEST_VAR_DIR, 'certificate.crt')
cacert = os.path.join(TEST_VAR_DIR, 'badca.crt')
try:
https.VerifiedHTTPSConnection('127.0.0.1', 0,
cert_file=cert_file,
cacert=cacert)
self.fail('Failed to raise assertion.')
except exc.SSLConfigurationError:
pass
def test_ssl_cert_cname(self):
"""Test certificate: CN match."""
cert_file = os.path.join(TEST_VAR_DIR, 'certificate.crt')
cert = crypto.load_certificate(crypto.FILETYPE_PEM,
open(cert_file).read())
# The expected cert should have CN=0.0.0.0
self.assertEqual('0.0.0.0', cert.get_subject().commonName)
try:
conn = https.VerifiedHTTPSConnection('0.0.0.0', 0)
https.do_verify_callback(None, cert, 0, 0, 1, host=conn.host)
except Exception:
self.fail('Unexpected exception.')
def test_ssl_cert_cname_wildcard(self):
"""Test certificate: wildcard CN match."""
cert_file = os.path.join(TEST_VAR_DIR, 'wildcard-certificate.crt')
cert = crypto.load_certificate(crypto.FILETYPE_PEM,
open(cert_file).read())
# The expected cert should have CN=*.pong.example.com
self.assertEqual('*.pong.example.com', cert.get_subject().commonName)
try:
conn = https.VerifiedHTTPSConnection('ping.pong.example.com', 0)
https.do_verify_callback(None, cert, 0, 0, 1, host=conn.host)
except Exception:
self.fail('Unexpected exception.')
def test_ssl_cert_subject_alt_name(self):
"""Test certificate: SAN match."""
cert_file = os.path.join(TEST_VAR_DIR, 'certificate.crt')
cert = crypto.load_certificate(crypto.FILETYPE_PEM,
open(cert_file).read())
# The expected cert should have CN=0.0.0.0
self.assertEqual('0.0.0.0', cert.get_subject().commonName)
try:
conn = https.VerifiedHTTPSConnection('alt1.example.com', 0)
https.do_verify_callback(None, cert, 0, 0, 1, host=conn.host)
except Exception:
self.fail('Unexpected exception.')
try:
conn = https.VerifiedHTTPSConnection('alt2.example.com', 0)
https.do_verify_callback(None, cert, 0, 0, 1, host=conn.host)
except Exception:
self.fail('Unexpected exception.')
def test_ssl_cert_subject_alt_name_wildcard(self):
"""Test certificate: wildcard SAN match."""
cert_file = os.path.join(TEST_VAR_DIR, 'wildcard-san-certificate.crt')
cert = crypto.load_certificate(crypto.FILETYPE_PEM,
open(cert_file).read())
# The expected cert should have CN=0.0.0.0
self.assertEqual('0.0.0.0', cert.get_subject().commonName)
try:
conn = https.VerifiedHTTPSConnection('alt1.example.com', 0)
https.do_verify_callback(None, cert, 0, 0, 1, host=conn.host)
except Exception:
self.fail('Unexpected exception.')
try:
conn = https.VerifiedHTTPSConnection('alt2.example.com', 0)
https.do_verify_callback(None, cert, 0, 0, 1, host=conn.host)
except Exception:
self.fail('Unexpected exception.')
try:
conn = https.VerifiedHTTPSConnection('alt3.example.net', 0)
https.do_verify_callback(None, cert, 0, 0, 1, host=conn.host)
self.fail('Failed to raise assertion.')
except exc.SSLCertificateError:
pass
def test_ssl_cert_mismatch(self):
"""Test certificate: bogus host."""
cert_file = os.path.join(TEST_VAR_DIR, 'certificate.crt')
cert = crypto.load_certificate(crypto.FILETYPE_PEM,
open(cert_file).read())
# The expected cert should have CN=0.0.0.0
self.assertEqual('0.0.0.0', cert.get_subject().commonName)
try:
conn = https.VerifiedHTTPSConnection('mismatch.example.com', 0)
except Exception:
self.fail('Failed to init VerifiedHTTPSConnection.')
self.assertRaises(exc.SSLCertificateError,
https.do_verify_callback, None, cert, 0, 0, 1,
host=conn.host)
def test_ssl_expired_cert(self):
"""Test certificate: out of date cert."""
cert_file = os.path.join(TEST_VAR_DIR, 'expired-cert.crt')
cert = crypto.load_certificate(crypto.FILETYPE_PEM,
open(cert_file).read())
# The expected expired cert has CN=openstack.example.com
self.assertEqual('openstack.example.com',
cert.get_subject().commonName)
try:
conn = https.VerifiedHTTPSConnection('openstack.example.com', 0)
except Exception:
raise
self.fail('Failed to init VerifiedHTTPSConnection.')
self.assertRaises(exc.SSLCertificateError,
https.do_verify_callback, None, cert, 0, 0, 1,
host=conn.host)
def test_ssl_broken_key_file(self):
"""Test verify exception is raised."""
cert_file = os.path.join(TEST_VAR_DIR, 'certificate.crt')
cacert = os.path.join(TEST_VAR_DIR, 'ca.crt')
key_file = 'fake.key'
self.assertRaises(
exc.SSLConfigurationError,
https.VerifiedHTTPSConnection, '127.0.0.1',
0, key_file=key_file,
cert_file=cert_file, cacert=cacert)
def test_ssl_init_ok_with_insecure_true(self):
"""Test VerifiedHTTPSConnection class init."""
key_file = os.path.join(TEST_VAR_DIR, 'privatekey.key')
cert_file = os.path.join(TEST_VAR_DIR, 'certificate.crt')
cacert = os.path.join(TEST_VAR_DIR, 'ca.crt')
try:
https.VerifiedHTTPSConnection(
'127.0.0.1', 0,
key_file=key_file,
cert_file=cert_file,
cacert=cacert, insecure=True)
except exc.SSLConfigurationError:
self.fail('Failed to init VerifiedHTTPSConnection.')
def test_ssl_init_ok_with_ssl_compression_false(self):
"""Test VerifiedHTTPSConnection class init."""
key_file = os.path.join(TEST_VAR_DIR, 'privatekey.key')
cert_file = os.path.join(TEST_VAR_DIR, 'certificate.crt')
cacert = os.path.join(TEST_VAR_DIR, 'ca.crt')
try:
https.VerifiedHTTPSConnection(
'127.0.0.1', 0,
key_file=key_file,
cert_file=cert_file,
cacert=cacert, ssl_compression=False)
except exc.SSLConfigurationError:
self.fail('Failed to init VerifiedHTTPSConnection.')
def test_ssl_init_non_byte_string(self):
"""Test VerifiedHTTPSConnection class non byte string.
Reproduces bug #1301849
"""
key_file = os.path.join(TEST_VAR_DIR, 'privatekey.key')
cert_file = os.path.join(TEST_VAR_DIR, 'certificate.crt')
cacert = os.path.join(TEST_VAR_DIR, 'ca.crt')
# Note: we reproduce on python 2.6/2.7, on 3.3 the bug doesn't occur.
key_file = key_file.encode('ascii', 'strict').decode('utf-8')
cert_file = cert_file.encode('ascii', 'strict').decode('utf-8')
cacert = cacert.encode('ascii', 'strict').decode('utf-8')
try:
https.VerifiedHTTPSConnection('127.0.0.1', 0,
key_file=key_file,
cert_file=cert_file,
cacert=cacert)
except exc.SSLConfigurationError:
self.fail('Failed to init VerifiedHTTPSConnection.')
class TestRequestsIntegration(testtools.TestCase):
def test_pool_patch(self):
client = http.HTTPClient("https://localhost",
ssl_compression=True)
self.assertNotEqual(https.HTTPSConnectionPool,
poolmanager.pool_classes_by_scheme["https"])
adapter = client.session.adapters.get("https://")
self.assertFalse(isinstance(adapter, https.HTTPSAdapter))
adapter = client.session.adapters.get("glance+https://")
self.assertFalse(isinstance(adapter, https.HTTPSAdapter))
def test_custom_https_adapter(self):
client = http.HTTPClient("https://localhost",
ssl_compression=False)
self.assertNotEqual(https.HTTPSConnectionPool,
poolmanager.pool_classes_by_scheme["https"])
adapter = client.session.adapters.get("https://")
self.assertFalse(isinstance(adapter, https.HTTPSAdapter))
adapter = client.session.adapters.get("glance+https://")
self.assertTrue(isinstance(adapter, https.HTTPSAdapter))
class TestHTTPSAdapter(testtools.TestCase):
def test__create_glance_httpsconnectionpool(self):
"""Regression test
Check that glanceclient's https pool is properly
configured without any weird exception.
"""
url = 'https://127.0.0.1:8000'
adapter = https.HTTPSAdapter()
try:
adapter._create_glance_httpsconnectionpool(url)
except Exception:
gc = Client('2', url,
insecure=False,
ssl_compression=False,
cert_file=cert_file,
cacert=cacert)
gc.images.get('image123')
except exc.CommunicationError as e:
if (six.PY2 and 'PrivateKey' not in e.message or
six.PY3 and 'No such file' not in e.message):
self.fail('No appropriate failure message is received')
except Exception as e:
self.fail('Unexpected exception has been raised')
@mock.patch('sys.stderr')
def test_v2_requests_bad_ca(self, __):
"""Test VerifiedHTTPSConnection: absence of SSL key file."""
port = self.port
url = 'https://0.0.0.0:%d' % port
cacert = os.path.join(TEST_VAR_DIR, 'badca.crt')
try:
gc = Client('2', url,
insecure=False,
ssl_compression=False,
cacert=cacert)
gc.images.get('image123')
except exc.CommunicationError as e:
if (six.PY2 and 'certificate' not in e.message or
six.PY3 and 'No such file' not in e.message):
self.fail('No appropriate failure message is received')
except Exception as e:
self.fail('Unexpected exception has been raised')

1
requirements.txt
View File

@@ -6,7 +6,6 @@ Babel>=1.3
argparse
PrettyTable<0.8,>=0.7
python-keystoneclient>=1.6.0
pyOpenSSL>=0.14
requests>=2.5.2
warlock<2,>=1.0.1
six>=1.9.0