From ef1ccdaca9512b88878155f7d8c2c77853d91252 Mon Sep 17 00:00:00 2001 From: Matt Riedemann Date: Mon, 16 Nov 2015 13:11:09 -0800 Subject: [PATCH] xen: mask passwords in volume connection_data dict The connection_data dict can have credentials in it, so we need to scrub those before putting the stringified dict into the StorageError message and raising that up and when logging the dict. Note that strutils.mask_password converts the dict to a string using six.text_type so we don't have to do that conversion first. SecurityImpact Change-Id: Ic5f4d4c26794550a92481bf2b725ef5eafa581b2 Closes-Bug: #1516765 (cherry picked from commit 8b289237ed6d53738c22878decf0c429301cf3d0) (cherry picked from commit cf197ec2d682fb4da777df2291ca7ef101f73b77) --- .../tests/unit/virt/xenapi/test_volume_utils.py | 17 +++++++++++++++-- nova/tests/unit/virt/xenapi/test_volumeops.py | 16 ++++++++++++++++ nova/virt/xenapi/volume_utils.py | 3 ++- nova/virt/xenapi/volumeops.py | 6 +++++- 4 files changed, 38 insertions(+), 4 deletions(-) diff --git a/nova/tests/unit/virt/xenapi/test_volume_utils.py b/nova/tests/unit/virt/xenapi/test_volume_utils.py index 7297690ecb2c..223a0141eec1 100644 --- a/nova/tests/unit/virt/xenapi/test_volume_utils.py +++ b/nova/tests/unit/virt/xenapi/test_volume_utils.py @@ -15,6 +15,7 @@ from eventlet import greenthread import mock +import six from nova import exception from nova import test @@ -168,14 +169,26 @@ class ParseVolumeInfoTestCase(stubs.XenAPITestBaseNoDB): 'target_lun': None, 'auth_method': 'CHAP', 'auth_username': 'username', - 'auth_password': 'password'}} + 'auth_password': 'verybadpass'}} def test_parse_volume_info_parsing_auth_details(self): conn_info = self._make_connection_info() result = volume_utils._parse_volume_info(conn_info['data']) self.assertEqual('username', result['chapuser']) - self.assertEqual('password', result['chappassword']) + self.assertEqual('verybadpass', result['chappassword']) + + def test_parse_volume_info_missing_details(self): + # Tests that a StorageError is raised if volume_id, target_host, or + # target_ign is missing from connection_data. Also ensures that the + # auth_password value is not present in the StorageError message. + for data_key_to_null in ('volume_id', 'target_portal', 'target_iqn'): + conn_info = self._make_connection_info() + conn_info['data'][data_key_to_null] = None + ex = self.assertRaises(exception.StorageError, + volume_utils._parse_volume_info, + conn_info['data']) + self.assertNotIn('verybadpass', six.text_type(ex)) def test_get_device_number_raise_exception_on_wrong_mountpoint(self): self.assertRaises( diff --git a/nova/tests/unit/virt/xenapi/test_volumeops.py b/nova/tests/unit/virt/xenapi/test_volumeops.py index 0e840bb20971..58c3fa51734f 100644 --- a/nova/tests/unit/virt/xenapi/test_volumeops.py +++ b/nova/tests/unit/virt/xenapi/test_volumeops.py @@ -381,6 +381,22 @@ class AttachVolumeTestCase(VolumeOpsTestBase): mock_intro.assert_called_once_with(self.session, "sr", target_lun="lun") + @mock.patch.object(volume_utils, "introduce_vdi") + @mock.patch.object(volumeops.LOG, 'debug') + def test_connect_hypervisor_to_volume_mask_password(self, mock_debug, + mock_intro): + # Tests that the connection_data is scrubbed before logging. + data = {'auth_password': 'verybadpass'} + self.ops._connect_hypervisor_to_volume("sr", data) + self.assertTrue(mock_debug.called, 'LOG.debug was not called') + password_logged = False + for call in mock_debug.call_args_list: + # The call object is a tuple of (args, kwargs) + if 'verybadpass' in call[0]: + password_logged = True + break + self.assertFalse(password_logged, 'connection_data was not scrubbed') + @mock.patch.object(vm_utils, "is_vm_shutdown") @mock.patch.object(vm_utils, "create_vbd") def test_attach_volume_to_vm_plug(self, mock_vbd, mock_shutdown): diff --git a/nova/virt/xenapi/volume_utils.py b/nova/virt/xenapi/volume_utils.py index c7bfe32ef8c2..af47e265515f 100644 --- a/nova/virt/xenapi/volume_utils.py +++ b/nova/virt/xenapi/volume_utils.py @@ -24,6 +24,7 @@ import string from eventlet import greenthread from oslo_config import cfg from oslo_log import log as logging +from oslo_utils import strutils from nova import exception from nova.i18n import _, _LE, _LW @@ -84,7 +85,7 @@ def _parse_volume_info(connection_data): target_iqn is None): raise exception.StorageError( reason=_('Unable to obtain target information %s') % - connection_data) + strutils.mask_password(connection_data)) volume_info = {} volume_info['id'] = volume_id volume_info['target'] = target_host diff --git a/nova/virt/xenapi/volumeops.py b/nova/virt/xenapi/volumeops.py index f816853c531d..b9e73e25f68c 100644 --- a/nova/virt/xenapi/volumeops.py +++ b/nova/virt/xenapi/volumeops.py @@ -19,6 +19,7 @@ Management class for Storage-related functions (attach, detach, etc). from oslo_log import log as logging from oslo_utils import excutils +from oslo_utils import strutils from nova import exception from nova.i18n import _LI, _LW @@ -91,7 +92,10 @@ class VolumeOps(object): return (sr_ref, sr_uuid) def _connect_hypervisor_to_volume(self, sr_ref, connection_data): - LOG.debug("Connect volume to hypervisor: %s", connection_data) + # connection_data can have credentials in it so make sure to scrub + # those before logging. + LOG.debug("Connect volume to hypervisor: %s", + strutils.mask_password(connection_data)) if 'vdi_uuid' in connection_data: vdi_ref = volume_utils.introduce_vdi( self._session, sr_ref,