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

diff --git a/cinder/tests/test_ibmnas.py b/cinder/tests/test_ibmnas.py
index 77bbcfc96a148f76247acf440420aa1e040edbde..fb1e8845f9b917432336b8ded08ad0d96e68abfe 100644 (file)
--- 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 c9be13b71b50ceebb4468a484eb528ef34090629..99e8fc7f39cfaa96da6da21e21ef0ec189c6e443 100644 (file)
--- 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 a9fe7f1adb8944dc92f65c4c3bf7ddaa36bf1452..54904c16701ee772ad25737cb574b1d4de50d0a9 100644 (file)
--- 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