From 3fff55681739872ab125aa7f0677f7d836c10615 Mon Sep 17 00:00:00 2001 From: Nilesh Bhosale Date: Sun, 24 Aug 2014 13:06:29 +0530 Subject: [PATCH] IBMNAS: Remove call to set r/w permissions to all During cinder volume create operation from a volume snapshot or from an existing volume (volume clone operation), the ibmnas driver sets 'rw' permissions to all, which is unnecessary and also poses security concerns. Fixing this issue, removing the calls to set rw permissions to all during these operations and adding a call to set 'rw' permissions only to the owner to make sure even if umask is set at the filesystem level, which might deny 'rw' access to the owner we explicitely set the required permissions on the volume file. Change-Id: I0e5ba9262a298e088f7724ddeda3537afa4b023e Closes-Bug: #1367238 --- cinder/tests/test_ibmnas.py | 57 +++++++++++++++-------------- cinder/volume/drivers/ibm/ibmnas.py | 5 +-- cinder/volume/drivers/remotefs.py | 4 ++ 3 files changed, 36 insertions(+), 30 deletions(-) diff --git a/cinder/tests/test_ibmnas.py b/cinder/tests/test_ibmnas.py index 77bbcfc96..fb1e8845f 100644 --- a/cinder/tests/test_ibmnas.py +++ b/cinder/tests/test_ibmnas.py @@ -196,7 +196,7 @@ class IBMNASDriverTestCase(test.TestCase): @mock.patch('cinder.volume.drivers.ibm.ibmnas.IBMNAS_NFSDriver.' '_ssh_operation') @mock.patch('cinder.openstack.common.processutils.execute') - def test_create_ibmnas_snap_nas_gpfs(self, mock_ssh, mock_execute): + def test_create_ibmnas_snap_nas_gpfs(self, mock_execute, mock_ssh): """Create ibmnas snap if mount point is provided.""" drv = self._driver @@ -291,8 +291,8 @@ class IBMNASDriverTestCase(test.TestCase): """Extend volume to greater size test case.""" drv = self._driver - mock_resize.return_value = True mock_local.return_value = self.TEST_LOCAL_PATH + mock_resize.return_value = True volume = FakeEnv() volume['name'] = 'vol-123' @@ -301,7 +301,7 @@ class IBMNASDriverTestCase(test.TestCase): @mock.patch('cinder.volume.drivers.ibm.ibmnas.IBMNAS_NFSDriver._run_ssh') @mock.patch('cinder.openstack.common.processutils.execute') - def test_delete_snapfiles(self, mock_ssh, mock_execute): + def test_delete_snapfiles(self, mock_execute, mock_ssh): """Delete_snapfiles test case.""" drv = self._driver @@ -309,15 +309,15 @@ class IBMNASDriverTestCase(test.TestCase): 'File name\n yes 0 /ibm/gpfs0/gshare/\n' 'volume-123\n EFSSG1000I The command' 'completed successfully.', '') - mock_execute.return_value = expected mock_ssh.return_value = expected + mock_execute.return_value = expected drv._delete_snapfiles(self.TEST_VOLUME_PATH, self.TEST_MNT_POINT) @mock.patch('cinder.volume.drivers.ibm.ibmnas.IBMNAS_NFSDriver._run_ssh') @mock.patch('cinder.openstack.common.processutils.execute') - def test_delete_snapfiles_nas_gpfs(self, mock_ssh, mock_execute): + def test_delete_snapfiles_nas_gpfs(self, mock_execute, mock_ssh): """Delete_snapfiles for gpfs-nas platform test case.""" drv = self._driver @@ -328,8 +328,8 @@ class IBMNASDriverTestCase(test.TestCase): '- ---------\n' 'yes 0\n' '/ibm/gpfs0/gshare/volume-123', '') - mock_execute.return_value = expected mock_ssh.return_value = expected + mock_execute.return_value = expected drv._delete_snapfiles(self.TEST_VOLUME_PATH, self.TEST_MNT_POINT) @@ -350,7 +350,7 @@ class IBMNASDriverTestCase(test.TestCase): '_get_export_path') @mock.patch('cinder.volume.drivers.ibm.ibmnas.IBMNAS_NFSDriver.' '_delete_snapfiles') - def test_delete_volume(self, mock_export, mock_snap): + def test_delete_volume(self, mock_snap, mock_export): """Delete volume test case.""" drv = self._driver @@ -372,10 +372,8 @@ class IBMNASDriverTestCase(test.TestCase): '_get_mount_point_for_share') @mock.patch('cinder.volume.drivers.ibm.ibmnas.IBMNAS_NFSDriver.' '_create_ibmnas_snap') - def test_create_snapshot(self, mock_export, - mock_provider, - mock_mount, - mock_snap): + def test_create_snapshot(self, mock_snap, mock_mount, mock_provider, + mock_export): """Create snapshot simple test case.""" drv = self._driver @@ -400,12 +398,12 @@ class IBMNASDriverTestCase(test.TestCase): @mock.patch('cinder.volume.drivers.ibm.ibmnas.IBMNAS_NFSDriver.' '_get_mount_point_for_share') @mock.patch('cinder.openstack.common.processutils.execute') - def test_delete_snapshot(self, mock_mount, mock_provider, mock_execute): + def test_delete_snapshot(self, mock_execute, mock_mount, mock_provider): """Delete snapshot simple test case.""" drv = self._driver - mock_mount.return_value = self.TEST_LOCAL_PATH mock_provider.return_value = self.TEST_VOLUME_PATH + mock_mount.return_value = self.TEST_LOCAL_PATH mock_execute.return_value = True volume = FakeEnv() @@ -424,20 +422,23 @@ class IBMNASDriverTestCase(test.TestCase): @mock.patch('cinder.volume.drivers.ibm.ibmnas.IBMNAS_NFSDriver.' '_create_ibmnas_copy') @mock.patch('cinder.volume.drivers.ibm.ibmnas.IBMNAS_NFSDriver.' - '_resize_volume_file') + '_find_share') @mock.patch('cinder.volume.drivers.ibm.ibmnas.IBMNAS_NFSDriver.local_path') @mock.patch('cinder.volume.drivers.ibm.ibmnas.IBMNAS_NFSDriver.' - '_find_share') + '_set_rw_permissions_for_owner') @mock.patch('cinder.volume.drivers.ibm.ibmnas.IBMNAS_NFSDriver.' - '_set_rw_permissions_for_all') - def test_create_cloned_volume(self, mock_export, mock_copy, - mock_resize, mock_local, - mock_find, mock_rw): + '_resize_volume_file') + def test_create_cloned_volume(self, mock_resize, mock_rw, mock_local, + mock_find, mock_copy, mock_export): """Clone volume with equal size test case.""" drv = self._driver mock_export.return_value = self.TEST_VOLUME_PATH - mock_copy.return_value = self.TEST_LOCAL_PATH + mock_copy.return_value = True + mock_find.return_value = self.TEST_LOCAL_PATH + mock_local.return_value = self.TEST_LOCAL_PATH + mock_rw.return_value = True + mock_resize.return_value = True volume_src = FakeEnv() volume_src['id'] = '123' @@ -458,22 +459,24 @@ class IBMNASDriverTestCase(test.TestCase): @mock.patch('cinder.volume.drivers.ibm.ibmnas.IBMNAS_NFSDriver.' '_create_ibmnas_snap') @mock.patch('cinder.volume.drivers.ibm.ibmnas.IBMNAS_NFSDriver.' - '_resize_volume_file') + '_find_share') @mock.patch('cinder.volume.drivers.ibm.ibmnas.IBMNAS_NFSDriver.local_path') @mock.patch('cinder.volume.drivers.ibm.ibmnas.IBMNAS_NFSDriver.' - '_find_share') + '_set_rw_permissions_for_owner') @mock.patch('cinder.volume.drivers.ibm.ibmnas.IBMNAS_NFSDriver.' - '_set_rw_permissions_for_all') - def test_create_volume_from_snapshot(self, mock_export, mock_snap, - mock_resize, mock_local, - mock_find, mock_rw): + '_resize_volume_file') + def test_create_volume_from_snapshot(self, mock_resize, mock_rw, + mock_local, mock_find, mock_snap, + mock_export): """Create volume from snapshot test case.""" drv = self._driver mock_export.return_value = '/export' mock_snap.return_value = self.TEST_LOCAL_PATH - mock_local.return_value = self.TEST_VOLUME_PATH mock_find.return_value = self.TEST_LOCAL_PATH + mock_local.return_value = self.TEST_VOLUME_PATH + mock_rw.return_value = True + mock_resize.return_value = True volume = FakeEnv() volume['id'] = '123' diff --git a/cinder/volume/drivers/ibm/ibmnas.py b/cinder/volume/drivers/ibm/ibmnas.py index c9be13b71..99e8fc7f3 100644 --- a/cinder/volume/drivers/ibm/ibmnas.py +++ b/cinder/volume/drivers/ibm/ibmnas.py @@ -343,7 +343,7 @@ class IBMNAS_NFSDriver(nfs.NfsDriver, san.SanDriver): volume['provider_location'] = self._find_share(volume['size']) volume_path = self.local_path(volume) - self._set_rw_permissions_for_all(volume_path) + self._set_rw_permissions_for_owner(volume_path) # Extend the volume if required self._resize_volume_file(volume_path, volume['size']) @@ -365,8 +365,7 @@ class IBMNAS_NFSDriver(nfs.NfsDriver, san.SanDriver): volume['provider_location'] = self._find_share(volume['size']) volume_path = self.local_path(volume) - - self._set_rw_permissions_for_all(volume_path) + self._set_rw_permissions_for_owner(volume_path) # Extend the volume if required self._resize_volume_file(volume_path, volume['size']) diff --git a/cinder/volume/drivers/remotefs.py b/cinder/volume/drivers/remotefs.py index a9fe7f1ad..54904c167 100644 --- a/cinder/volume/drivers/remotefs.py +++ b/cinder/volume/drivers/remotefs.py @@ -229,6 +229,10 @@ class RemoteFSDriver(driver.VolumeDriver): """Sets 666 permissions for the path.""" self._execute('chmod', 'ugo+rw', path, run_as_root=True) + def _set_rw_permissions_for_owner(self, path): + """Sets read-write permissions to the owner for the path.""" + self._execute('chmod', 'u+rw', path, run_as_root=True) + def local_path(self, volume): """Get volume path (mounted locally fs path) for given volume :param volume: volume reference -- 2.45.2