From: Ryan McNair Date: Thu, 27 Aug 2015 20:51:14 +0000 (+0000) Subject: Fix MITM vulnerability for Brocade FC SAN lookup X-Git-Url: https://review.fuel-infra.org/gitweb?a=commitdiff_plain;h=46fdb37c68b605d26f98ac673ebe4e70c39b4b99;p=openstack-build%2Fcinder-build.git Fix MITM vulnerability for Brocade FC SAN lookup Refactor the Brocade FC SAN lookup service implementation to use common SSH utilities which already avoid MITM vulnerabilities. This is a follow-up to https://review.openstack.org/#/c/138526/ which reverted an incomplete fix for the MITM issues for Brocade. Change-Id: I2d87b55f56f08208f0da11cac40682d51da5b536 Closes-Bug: #1391311 --- diff --git a/cinder/tests/unit/zonemanager/test_brcd_fc_san_lookup_service.py b/cinder/tests/unit/zonemanager/test_brcd_fc_san_lookup_service.py index 03192072d..df208ee35 100644 --- a/cinder/tests/unit/zonemanager/test_brcd_fc_san_lookup_service.py +++ b/cinder/tests/unit/zonemanager/test_brcd_fc_san_lookup_service.py @@ -20,10 +20,11 @@ """Unit tests for brcd fc san lookup service.""" import mock +from oslo_concurrency import processutils as putils from oslo_config import cfg -import paramiko from cinder import exception +from cinder import ssh_utils from cinder import test from cinder.volume import configuration as conf import cinder.zonemanager.drivers.brocade.brcd_fc_san_lookup_service \ @@ -31,14 +32,41 @@ import cinder.zonemanager.drivers.brocade.brcd_fc_san_lookup_service \ from cinder.zonemanager.drivers.brocade import fc_zone_constants -nsshow = '20:1a:00:05:1e:e8:e3:29' -switch_data = [' N 011a00;2,3;20:1a:00:05:1e:e8:e3:29;\ - 20:1a:00:05:1e:e8:e3:29;na'] -nsshow_data = ['10:00:8c:7c:ff:52:3b:01', '20:24:00:02:ac:00:0a:50'] +parsed_switch_port_wwns = ['20:1a:00:05:1e:e8:e3:29', + '10:00:00:90:fa:34:40:f6'] +switch_data = (""" + Type Pid COS PortName NodeName TTL(sec) + N 011a00; 2,3; %(port_1)s; 20:1a:00:05:1e:e8:e3:29; na + FC4s: FCP + PortSymb: [26] "222222 - 1:1:1 - LPe12442" + NodeSymb: [32] "SomeSym 7211" + Fabric Port Name: 20:1a:00:05:1e:e8:e3:29 + Permanent Port Name: 22:22:00:22:ac:00:bc:b0 + Port Index: 0 + Share Area: No + Device Shared in Other AD: No + Redirect: No + Partial: No + LSAN: No + N 010100; 2,3; %(port_2)s; 20:00:00:00:af:00:00:af; na + FC4s: FCP + PortSymb: [26] "333333 - 1:1:1 - LPe12442" + NodeSymb: [32] "SomeSym 2222" + Fabric Port Name: 10:00:00:90:fa:34:40:f6 + Permanent Port Name: 22:22:00:22:ac:00:bc:b0 + Port Index: 0 + Share Area: No + Device Shared in Other AD: No + Redirect: No + Partial: No + LSAN: No""" % {'port_1': parsed_switch_port_wwns[0], + 'port_2': parsed_switch_port_wwns[1]}) + _device_map_to_verify = { 'BRCD_FAB_2': { - 'initiator_port_wwn_list': ['10008c7cff523b01'], - 'target_port_wwn_list': ['20240002ac000a50']}} + 'initiator_port_wwn_list': [parsed_switch_port_wwns[1].replace(':', + '')], + 'target_port_wwn_list': [parsed_switch_port_wwns[0].replace(':', '')]}} class TestBrcdFCSanLookupService(brcd_lookup.BrcdFCSanLookupService, @@ -46,7 +74,6 @@ class TestBrcdFCSanLookupService(brcd_lookup.BrcdFCSanLookupService, def setUp(self): super(TestBrcdFCSanLookupService, self).setUp() - self.client = paramiko.SSHClient() self.configuration = conf.Configuration(None) self.configuration.set_default('fc_fabric_names', 'BRCD_FAB_2', 'fc-zone-manager') @@ -73,57 +100,47 @@ class TestBrcdFCSanLookupService(brcd_lookup.BrcdFCSanLookupService, config = conf.Configuration(fc_fabric_opts, 'BRCD_FAB_2') self.fabric_configs = {'BRCD_FAB_2': config} - @mock.patch.object(paramiko.hostkeys.HostKeys, 'load') - def test_create_ssh_client(self, load_mock): - mock_args = {} - mock_args['known_hosts_file'] = 'dummy_host_key_file' - mock_args['missing_key_policy'] = paramiko.RejectPolicy() - ssh_client = self.create_ssh_client(**mock_args) - self.assertEqual('dummy_host_key_file', ssh_client._host_keys_filename) - self.assertTrue(isinstance(ssh_client._policy, paramiko.RejectPolicy)) - mock_args = {} - ssh_client = self.create_ssh_client(**mock_args) - self.assertIsNone(ssh_client._host_keys_filename) - self.assertTrue(isinstance(ssh_client._policy, paramiko.WarningPolicy)) - @mock.patch.object(brcd_lookup.BrcdFCSanLookupService, 'get_nameserver_info') - def test_get_device_mapping_from_network(self, get_nameserver_info_mock): - initiator_list = ['10008c7cff523b01'] - target_list = ['20240002ac000a50', '20240002ac000a40'] - with mock.patch.object(self.client, 'connect'): - get_nameserver_info_mock.return_value = (nsshow_data) - device_map = self.get_device_mapping_from_network( - initiator_list, target_list) - self.assertDictMatch(device_map, _device_map_to_verify) + @mock.patch('cinder.zonemanager.drivers.brocade.brcd_fc_san_lookup_service' + '.ssh_utils.SSHPool') + def test_get_device_mapping_from_network(self, mock_ssh_pool, + get_nameserver_info_mock): + initiator_list = [parsed_switch_port_wwns[1]] + target_list = [parsed_switch_port_wwns[0], '20240002ac000a40'] + get_nameserver_info_mock.return_value = parsed_switch_port_wwns + device_map = self.get_device_mapping_from_network( + initiator_list, target_list) + self.assertDictMatch(device_map, _device_map_to_verify) @mock.patch.object(brcd_lookup.BrcdFCSanLookupService, '_get_switch_data') def test_get_nameserver_info(self, get_switch_data_mock): ns_info_list = [] - ns_info_list_expected = ['20:1a:00:05:1e:e8:e3:29', - '20:1a:00:05:1e:e8:e3:29'] + get_switch_data_mock.return_value = (switch_data) - ns_info_list = self.get_nameserver_info() - self.assertEqual(ns_info_list_expected, ns_info_list) + # get_switch_data will be called twice with the results appended + ns_info_list_expected = (parsed_switch_port_wwns + + parsed_switch_port_wwns) - def test__get_switch_data(self): - cmd = fc_zone_constants.NS_SHOW + ns_info_list = self.get_nameserver_info(None) + self.assertEqual(ns_info_list_expected, ns_info_list) - with mock.patch.object(self.client, 'exec_command') \ - as exec_command_mock: - exec_command_mock.return_value = (Stream(), - Stream(nsshow), - Stream()) - switch_data = self._get_switch_data(cmd) - self.assertEqual(nsshow, switch_data) - exec_command_mock.assert_called_once_with(cmd) + @mock.patch.object(putils, 'ssh_execute', return_value=(switch_data, '')) + @mock.patch.object(ssh_utils.SSHPool, 'item') + def test__get_switch_data(self, ssh_pool_mock, ssh_execute_mock): + actual_switch_data = self._get_switch_data(ssh_pool_mock, + fc_zone_constants.NS_SHOW) + self.assertEqual(actual_switch_data, switch_data) + ssh_execute_mock.side_effect = putils.ProcessExecutionError() + self.assertRaises(exception.FCSanLookupServiceException, + self._get_switch_data, ssh_pool_mock, + fc_zone_constants.NS_SHOW) def test__parse_ns_output(self): - invalid_switch_data = [' N 011a00;20:1a:00:05:1e:e8:e3:29'] + invalid_switch_data = ' N 011a00;20:1a:00:05:1e:e8:e3:29' return_wwn_list = [] - expected_wwn_list = ['20:1a:00:05:1e:e8:e3:29'] return_wwn_list = self._parse_ns_output(switch_data) - self.assertEqual(expected_wwn_list, return_wwn_list) + self.assertEqual(parsed_switch_port_wwns, return_wwn_list) self.assertRaises(exception.InvalidParameterValue, self._parse_ns_output, invalid_switch_data) @@ -133,23 +150,3 @@ class TestBrcdFCSanLookupService(brcd_lookup.BrcdFCSanLookupService, expected_wwn_list = ['10:00:8c:7c:ff:52:3b:01'] return_wwn_list.append(self.get_formatted_wwn(wwn_list[0])) self.assertEqual(expected_wwn_list, return_wwn_list) - - -class Channel(object): - def recv_exit_status(self): - return 0 - - -class Stream(object): - def __init__(self, buffer=''): - self.buffer = buffer - self.channel = Channel() - - def readlines(self): - return self.buffer - - def close(self): - pass - - def flush(self): - self.buffer = '' diff --git a/cinder/zonemanager/drivers/brocade/brcd_fc_san_lookup_service.py b/cinder/zonemanager/drivers/brocade/brcd_fc_san_lookup_service.py index b5246e279..3e4059963 100644 --- a/cinder/zonemanager/drivers/brocade/brcd_fc_san_lookup_service.py +++ b/cinder/zonemanager/drivers/brocade/brcd_fc_san_lookup_service.py @@ -16,13 +16,14 @@ # under the License. # +from oslo_concurrency import processutils from oslo_log import log as logging from oslo_utils import excutils -import paramiko import six from cinder import exception from cinder.i18n import _, _LE +from cinder import ssh_utils from cinder import utils from cinder.zonemanager.drivers.brocade import brcd_fabric_opts as fabric_opts import cinder.zonemanager.drivers.brocade.fc_zone_constants as zone_constant @@ -46,7 +47,6 @@ class BrcdFCSanLookupService(fc_service.FCSanLookupService): super(BrcdFCSanLookupService, self).__init__(**kwargs) self.configuration = kwargs.get('configuration', None) self.create_configuration() - self.client = self.create_ssh_client(**kwargs) def create_configuration(self): """Configuration specific to SAN context values.""" @@ -61,19 +61,6 @@ class BrcdFCSanLookupService(fc_service.FCSanLookupService): self.fabric_configs = fabric_opts.load_fabric_configurations( fabric_names) - def create_ssh_client(self, **kwargs): - ssh_client = paramiko.SSHClient() - known_hosts_file = kwargs.get('known_hosts_file', None) - if known_hosts_file is None: - ssh_client.load_system_host_keys() - else: - ssh_client.load_host_keys(known_hosts_file) - missing_key_policy = kwargs.get('missing_key_policy', None) - if missing_key_policy is None: - missing_key_policy = paramiko.WarningPolicy() - ssh_client.set_missing_host_key_policy(missing_key_policy) - return ssh_client - def get_device_mapping_from_network(self, initiator_wwn_list, target_wwn_list): @@ -126,15 +113,16 @@ class BrcdFCSanLookupService(fc_service.FCSanLookupService): fabric_port = self.fabric_configs[fabric_name].safe_get( 'fc_fabric_port') + ssh_pool = ssh_utils.SSHPool(fabric_ip, fabric_port, None, + fabric_user, password=fabric_pwd) + # Get name server data from fabric and find the targets # logged in nsinfo = '' try: LOG.debug("Getting name server data for " "fabric %s", fabric_ip) - self.client.connect( - fabric_ip, fabric_port, fabric_user, fabric_pwd) - nsinfo = self.get_nameserver_info() + nsinfo = self.get_nameserver_info(ssh_pool) except exception.FCSanLookupServiceException: with excutils.save_and_reraise_exception(): LOG.error(_LE("Failed collecting name server info from" @@ -145,8 +133,7 @@ class BrcdFCSanLookupService(fc_service.FCSanLookupService): ) % {'fabric': fabric_ip, 'err': e} LOG.error(msg) raise exception.FCSanLookupServiceException(message=msg) - finally: - self.client.close() + LOG.debug("Lookup service:nsinfo-%s", nsinfo) LOG.debug("Lookup service:initiator list from " "caller-%s", formatted_initiator_list) @@ -184,23 +171,28 @@ class BrcdFCSanLookupService(fc_service.FCSanLookupService): LOG.debug("Device map for SAN context: %s", device_map) return device_map - def get_nameserver_info(self): + def get_nameserver_info(self, ssh_pool): """Get name server data from fabric. This method will return the connected node port wwn list(local and remote) for the given switch fabric + + :param ssh_pool: SSH connections for the current fabric """ cli_output = None nsinfo_list = [] try: - cli_output = self._get_switch_data(zone_constant.NS_SHOW) + cli_output = self._get_switch_data(ssh_pool, + zone_constant.NS_SHOW) except exception.FCSanLookupServiceException: with excutils.save_and_reraise_exception(): LOG.error(_LE("Failed collecting nsshow info for fabric")) if cli_output: nsinfo_list = self._parse_ns_output(cli_output) try: - cli_output = self._get_switch_data(zone_constant.NS_CAM_SHOW) + cli_output = self._get_switch_data(ssh_pool, + zone_constant.NS_CAM_SHOW) + except exception.FCSanLookupServiceException: with excutils.save_and_reraise_exception(): LOG.error(_LE("Failed collecting nscamshow")) @@ -209,26 +201,19 @@ class BrcdFCSanLookupService(fc_service.FCSanLookupService): LOG.debug("Connector returning nsinfo-%s", nsinfo_list) return nsinfo_list - def _get_switch_data(self, cmd): - stdin, stdout, stderr = None, None, None + def _get_switch_data(self, ssh_pool, cmd): utils.check_ssh_injection([cmd]) - try: - stdin, stdout, stderr = self.client.exec_command(cmd) - switch_data = stdout.readlines() - except paramiko.SSHException as e: - msg = (_("SSH Command failed with error '%(err)s' " - "'%(command)s'") % {'err': six.text_type(e), - 'command': cmd}) - LOG.error(msg) - raise exception.FCSanLookupServiceException(message=msg) - finally: - if (stdin): - stdin.flush() - stdin.close() - if (stdout): - stdout.close() - if (stderr): - stderr.close() + + with ssh_pool.item() as ssh: + try: + switch_data, err = processutils.ssh_execute(ssh, cmd) + except processutils.ProcessExecutionError as e: + msg = (_("SSH Command failed with error: '%(err)s', Command: " + "'%(command)s'") % {'err': six.text_type(e), + 'command': cmd}) + LOG.error(msg) + raise exception.FCSanLookupServiceException(message=msg) + return switch_data def _parse_ns_output(self, switch_data): @@ -239,12 +224,13 @@ class BrcdFCSanLookupService(fc_service.FCSanLookupService): :returns list of device port wwn from ns info """ nsinfo_list = [] - for line in switch_data: + lines = switch_data.split('\n') + for line in lines: if not(" NL " in line or " N " in line): continue linesplit = line.split(';') if len(linesplit) > 2: - node_port_wwn = linesplit[2] + node_port_wwn = linesplit[2].strip() nsinfo_list.append(node_port_wwn) else: msg = _("Malformed nameserver string: %s") % line