From 264b4a2523c165640f17aa4837f87ddfd0b49640 Mon Sep 17 00:00:00 2001 From: Daniel Gollub Date: Sun, 2 Mar 2014 09:33:38 +0100 Subject: [PATCH] Replace HTTPSConnection in NEC plugin Replace HTTPSConnection in NEC plugin PFC driver with Requests. SSL Verification is from now on enabled by default. This changes the default behaviour and is the primary intention of this change: verify SSL certificates. This might break existing configuration/setups where the SSL certificate used by the NEC PFC driver would not pass the verification. SecurityImpact DocImpact Partial-Bug: 1188189 Change-Id: I1e5fdc9c2ed5b812aa6509d1639bd499acc5c337 --- etc/neutron/plugins/nec/nec.ini | 3 + neutron/plugins/nec/common/config.py | 2 + neutron/plugins/nec/common/ofc_client.py | 72 ++++++++++---------- neutron/plugins/nec/drivers/pfc.py | 3 +- neutron/tests/unit/nec/test_ofc_client.py | 80 +++++++++++------------ neutron/tests/unit/nec/test_pfc_driver.py | 1 + 6 files changed, 82 insertions(+), 79 deletions(-) diff --git a/etc/neutron/plugins/nec/nec.ini b/etc/neutron/plugins/nec/nec.ini index b2cb3dbfa..aa4171da7 100644 --- a/etc/neutron/plugins/nec/nec.ini +++ b/etc/neutron/plugins/nec/nec.ini @@ -45,6 +45,9 @@ firewall_driver = neutron.agent.linux.iptables_firewall.OVSHybridIptablesFirewal # Certificate file # cert_file = +# Disable SSL certificate verification +# insecure_ssl = false + # Maximum attempts per OFC API request. NEC plugin retries # API request to OFC when OFC returns ServiceUnavailable (503). # The value must be greater than 0. diff --git a/neutron/plugins/nec/common/config.py b/neutron/plugins/nec/common/config.py index 21c5e1824..80cfd0200 100644 --- a/neutron/plugins/nec/common/config.py +++ b/neutron/plugins/nec/common/config.py @@ -51,6 +51,8 @@ ofc_opts = [ help=_("Key file")), cfg.StrOpt('cert_file', default=None, help=_("Certificate file")), + cfg.BoolOpt('insecure_ssl', default=False, + help=_("Disable SSL certificate verification")), cfg.IntOpt('api_max_attempts', default=3, help=_("Maximum attempts per OFC API request." "NEC plugin retries API request to OFC " diff --git a/neutron/plugins/nec/common/ofc_client.py b/neutron/plugins/nec/common/ofc_client.py index 957f24483..d51738c7a 100644 --- a/neutron/plugins/nec/common/ofc_client.py +++ b/neutron/plugins/nec/common/ofc_client.py @@ -15,11 +15,11 @@ # under the License. # @author: Ryota MIBU -import httplib import json -import socket import time +import requests + from neutron.openstack.common import excutils from neutron.openstack.common import log as logging from neutron.plugins.nec.common import config @@ -33,7 +33,7 @@ class OFCClient(object): """A HTTP/HTTPS client for OFC Drivers.""" def __init__(self, host="127.0.0.1", port=8888, use_ssl=False, - key_file=None, cert_file=None): + key_file=None, cert_file=None, insecure_ssl=False): """Creates a new client to some OFC. :param host: The host where service resides @@ -41,35 +41,40 @@ class OFCClient(object): :param use_ssl: True to use SSL, False to use HTTP :param key_file: The SSL key file to use if use_ssl is true :param cert_file: The SSL cert file to use if use_ssl is true + :param insecure_ssl: Don't verify SSL certificate """ self.host = host self.port = port self.use_ssl = use_ssl self.key_file = key_file self.cert_file = cert_file + self.insecure_ssl = insecure_ssl self.connection = None - def get_connection(self): - """Returns the proper connection.""" - if self.use_ssl: - connection_type = httplib.HTTPSConnection - else: - connection_type = httplib.HTTPConnection - - # Open connection and send request, handling SSL certs - certs = {'key_file': self.key_file, 'cert_file': self.cert_file} - certs = dict((x, certs[x]) for x in certs if certs[x] is not None) - if self.use_ssl and len(certs): - conn = connection_type(self.host, self.port, **certs) - else: - conn = connection_type(self.host, self.port) - return conn - def _format_error_message(self, status, detail): detail = ' ' + detail if detail else '' return (_("Operation on OFC failed: %(status)s%(msg)s") % {'status': status, 'msg': detail}) + def _get_response(self, method, action, body=None): + headers = {"Content-Type": "application/json"} + protocol = "http" + certs = {'key_file': self.key_file, 'cert_file': self.cert_file} + certs = dict((x, certs[x]) for x in certs if certs[x] is not None) + verify = True + + if self.use_ssl: + protocol = "https" + if self.insecure_ssl: + verify = False + + url = "%s://%s:%d%s" % (protocol, self.host, int(self.port), + action) + + res = requests.request(method, url, data=body, headers=headers, + cert=certs, verify=verify) + return res + def do_single_request(self, method, action, body=None): action = config.OFC.path_prefix + action LOG.debug(_("Client request: %(host)s:%(port)s " @@ -79,13 +84,10 @@ class OFCClient(object): if type(body) is dict: body = json.dumps(body) try: - conn = self.get_connection() - headers = {"Content-Type": "application/json"} - conn.request(method, action, body, headers) - res = conn.getresponse() - data = res.read() + res = self._get_response(method, action, body) + data = res.text LOG.debug(_("OFC returns [%(status)s:%(data)s]"), - {'status': res.status, + {'status': res.status_code, 'data': data}) # Try to decode JSON data if possible. @@ -94,33 +96,33 @@ class OFCClient(object): except (ValueError, TypeError): pass - if res.status in (httplib.OK, - httplib.CREATED, - httplib.ACCEPTED, - httplib.NO_CONTENT): + if res.status_code in (requests.codes.OK, + requests.codes.CREATED, + requests.codes.ACCEPTED, + requests.codes.NO_CONTENT): return data - elif res.status == httplib.SERVICE_UNAVAILABLE: - retry_after = res.getheader('retry-after') + elif res.status_code == requests.codes.SERVICE_UNAVAILABLE: + retry_after = res.headers.get('retry-after') LOG.warning(_("OFC returns ServiceUnavailable " "(retry-after=%s)"), retry_after) raise nexc.OFCServiceUnavailable(retry_after=retry_after) - elif res.status == httplib.NOT_FOUND: + elif res.status_code == requests.codes.NOT_FOUND: LOG.info(_("Specified resource %s does not exist on OFC "), action) raise nexc.OFCResourceNotFound(resource=action) else: LOG.warning(_("Operation on OFC failed: " "status=%(status)s, detail=%(detail)s"), - {'status': res.status, 'detail': data}) + {'status': res.status_code, 'detail': data}) params = {'reason': _("Operation on OFC failed"), - 'status': res.status} + 'status': res.status_code} if isinstance(data, dict): params['err_code'] = data.get('err_code') params['err_msg'] = data.get('err_msg') else: params['err_msg'] = data raise nexc.OFCException(**params) - except (socket.error, IOError) as e: + except requests.exceptions.RequestException as e: reason = _("Failed to connect OFC : %s") % e LOG.error(reason) raise nexc.OFCException(reason=reason) diff --git a/neutron/plugins/nec/drivers/pfc.py b/neutron/plugins/nec/drivers/pfc.py index deae83354..85921b712 100644 --- a/neutron/plugins/nec/drivers/pfc.py +++ b/neutron/plugins/nec/drivers/pfc.py @@ -57,7 +57,8 @@ class PFCDriverBase(ofc_driver_base.OFCDriverBase): port=conf_ofc.port, use_ssl=conf_ofc.use_ssl, key_file=conf_ofc.key_file, - cert_file=conf_ofc.cert_file) + cert_file=conf_ofc.cert_file, + insecure_ssl=conf_ofc.insecure_ssl) @classmethod def filter_supported(cls): diff --git a/neutron/tests/unit/nec/test_ofc_client.py b/neutron/tests/unit/nec/test_ofc_client.py index 68668d4d4..09a214a1f 100644 --- a/neutron/tests/unit/nec/test_ofc_client.py +++ b/neutron/tests/unit/nec/test_ofc_client.py @@ -17,10 +17,10 @@ # @author: Akihiro Motoki import json -import socket import mock from oslo.config import cfg +import requests from neutron.plugins.nec.common import config from neutron.plugins.nec.common import exceptions as nexc @@ -28,19 +28,25 @@ from neutron.plugins.nec.common import ofc_client from neutron.tests import base +class FakeResponse(requests.Response): + def __init__(self, status_code=None, text=None, headers=None): + self._text = text + self.status_code = status_code + if headers is not None: + self.headers = headers + + @property + def text(self): + return self._text + + class OFCClientTest(base.BaseTestCase): def _test_do_request(self, status, resbody, expected_data, exctype=None, exc_checks=None, path_prefix=None): - res = mock.Mock() - res.status = status - res.read.return_value = resbody + req = mock.Mock(return_value=(FakeResponse(status, resbody))) - conn = mock.Mock() - conn.getresponse.return_value = res - - with mock.patch.object(ofc_client.OFCClient, 'get_connection', - return_value=conn): + with mock.patch.object(requests, 'request', req): client = ofc_client.OFCClient() path = '/somewhere' realpath = path_prefix + path if path_prefix else path @@ -56,11 +62,9 @@ class OFCClientTest(base.BaseTestCase): self.assertEqual(response, expected_data) headers = {"Content-Type": "application/json"} - expected = [ - mock.call.request('GET', realpath, '{}', headers), - mock.call.getresponse(), - ] - conn.assert_has_calls(expected) + req.assert_called_with('GET', 'http://127.0.0.1:8888' + realpath, + verify=True, cert={}, data='{}', + headers=headers) def test_do_request_200_json_value(self): self._test_do_request(200, json.dumps([1, 2, 3]), [1, 2, 3]) @@ -108,13 +112,12 @@ class OFCClientTest(base.BaseTestCase): exc_checks) def test_do_request_socket_error(self): - conn = mock.Mock() - conn.request.side_effect = socket.error - data = _("An OFC exception has occurred: Failed to connect OFC : ") - with mock.patch.object(ofc_client.OFCClient, 'get_connection', - return_value=conn): + req = mock.Mock() + req.side_effect = requests.exceptions.RequestException + + with mock.patch.object(requests, 'request', req): client = ofc_client.OFCClient() e = self.assertRaises(nexc.OFCException, client.do_request, @@ -124,10 +127,9 @@ class OFCClientTest(base.BaseTestCase): self.assertIsNone(getattr(e, k)) headers = {"Content-Type": "application/json"} - expected = [ - mock.call.request('GET', '/somewhere', '{}', headers), - ] - conn.assert_has_calls(expected) + req.assert_called_with('GET', 'http://127.0.0.1:8888/somewhere', + verify=True, cert={}, data='{}', + headers=headers) def test_do_request_retry_fail_after_one_attempts(self): self._test_do_request_retry_after(1, api_max_attempts=1) @@ -148,24 +150,17 @@ class OFCClientTest(base.BaseTestCase): cfg.CONF.set_override('api_max_attempts', api_max_attempts, group='OFC') - res_unavail = mock.Mock() - res_unavail.status = 503 - res_unavail.read.return_value = None - res_unavail.getheader.return_value = '10' + res_unavail = FakeResponse(503, headers={'retry-after': '10'}) + res_ok = FakeResponse(200) - res_ok = mock.Mock() - res_ok.status = 200 - res_ok.read.return_value = None - - conn = mock.Mock() + req = mock.Mock() if succeed_final: - side_effect = [res_unavail] * (exp_request_count - 1) + [res_ok] + req.side_effect = ([res_unavail] * (exp_request_count - 1) + + [res_ok]) else: - side_effect = [res_unavail] * exp_request_count - conn.getresponse.side_effect = side_effect + req.side_effect = [res_unavail] * exp_request_count - with mock.patch.object(ofc_client.OFCClient, 'get_connection', - return_value=conn): + with mock.patch.object(requests, 'request', req): with mock.patch('time.sleep') as sleep: client = ofc_client.OFCClient() if succeed_final: @@ -176,11 +171,10 @@ class OFCClientTest(base.BaseTestCase): client.do_request, 'GET', '/somewhere') self.assertEqual('10', e.retry_after) + headers = {"Content-Type": "application/json"} - expected = [ - mock.call.request('GET', '/somewhere', None, headers), - mock.call.getresponse(), - ] * exp_request_count - conn.assert_has_calls(expected) - self.assertEqual(exp_request_count, conn.request.call_count) + req.assert_called_with('GET', 'http://127.0.0.1:8888/somewhere', + verify=True, cert={}, data=None, + headers=headers) + self.assertEqual(exp_request_count, req.call_count) self.assertEqual(exp_request_count - 1, sleep.call_count) diff --git a/neutron/tests/unit/nec/test_pfc_driver.py b/neutron/tests/unit/nec/test_pfc_driver.py index f5b27036d..c198800c4 100644 --- a/neutron/tests/unit/nec/test_pfc_driver.py +++ b/neutron/tests/unit/nec/test_pfc_driver.py @@ -39,6 +39,7 @@ class TestConfig(object): use_ssl = False key_file = None cert_file = None + insecure_ssl = False def _ofc(id):