CA-371791: Fix world readable permissions on EXTSR

FileSR.attach modified to change the mount point permissions to 700.
This prevents any non-root user from reading the contents of the SR.
EXTSR.attach modified to call FileSR.attach.
Other SRs could be changed to reuse FileSR.attach in the future.
Unit tests added for FileSR to test old and new attach functionality.

Signed-off-by: Kevin Lampis <klampis@cloud.com>

drivers/EXTSR.py

@@ -108,10 +108,6 @@ def attach(self, sr_uuid):
                 #Activate LV
                 cmd = ['lvchange', '-ay', self.remotepath]
                 util.pread2(cmd)
-
-                # make a mountpoint:
-                if not os.path.isdir(self.path):
-                    os.makedirs(self.path)
             except util.CommandException as inst:
                 raise xs_errors.XenError(
                     'LVMMount',
@@ -127,12 +123,7 @@ def attach(self, sr_uuid):
                         'LVMMount',
                         opterr='FSCK failed on %s. Errno is %d' % (self.remotepath, inst.code))
 
-            try:
-                util.pread(["mount", self.remotepath, self.path])
-            except util.CommandException as inst:
-                raise xs_errors.XenError(
-                    'LVMMount',
-                    opterr='Failed to mount FS. Errno is %d' % inst.code)
+            super(EXTSR, self).attach(sr_uuid, bind=False)
 
         self.attached = True
 

drivers/FileSR.py

@@ -156,16 +156,20 @@ def delete(self, sr_uuid):
             raise xs_errors.XenError('FileSRDelete', \
                   opterr='error %d' % inst.code)
 
-    def attach(self, sr_uuid):
+    def attach(self, sr_uuid, bind=True):
         if not self._checkmount():
             try:
-                util.ioretry(lambda: util.makedirs(self.path))
+                util.ioretry(lambda: util.makedirs(self.path, mode=0o700))
             except util.CommandException as inst:
                 if inst.code != errno.EEXIST:
                     raise xs_errors.XenError("FileSRCreate", \
                                              opterr='fail to create mount point. Errno is %s' % inst.code)
             try:
-                util.pread(["mount", "--bind", self.remotepath, self.path])
+                cmd = ["mount", self.remotepath, self.path]
+                if bind:
+                    cmd.append("--bind")
+                util.pread(cmd)
+                os.chmod(self.path, mode=0o0700)
             except util.CommandException as inst:
                 raise xs_errors.XenError('FileSRCreate', \
                                          opterr='fail to mount FileSR. Errno is %s' % inst.code)

tests/test_FileSR.py

@@ -7,10 +7,12 @@
 import xmlrpc.client
 
 from xml.dom.minidom import parseString
+import EXTSR
 
 import FileSR
 import SR
 import SRCommand
+import testlib
 import util
 import vhdutil
 from XenAPI import Failure
@@ -570,3 +572,150 @@ def test_scan_load_vdis_scan_list_differ(self):
 
         # Assert
         self.assertEqual(1, len(test_sr.vdis))
+
+class TestFileSR(unittest.TestCase):
+    def setUp(self):
+        pread_patcher = mock.patch('FileSR.util.pread')
+        self.mock_pread = pread_patcher.start()
+
+        errors_patcher = mock.patch('FileSR.xs_errors.XML_DEFS',
+                "drivers/XE_SR_ERRORCODES.xml")
+        errors_patcher.start()
+
+        sr_init_patcher = mock.patch('SR.SR.__init__')
+        def fake_sr_init(self, srcmd, sr_uuid):
+            self.sr_ref = False
+        self.mock_sr_init = sr_init_patcher.start()
+        self.mock_sr_init.side_effect = fake_sr_init
+
+        checkmount_patcher = mock.patch('FileSR.FileSR._checkmount')
+        self.mock_filesr_checkmount = checkmount_patcher.start()
+        self.mock_filesr_checkmount.return_value = False
+
+    def test_attach_does_nothing_if_already_mounted(self):
+
+        self.mock_filesr_checkmount.return_value = True
+        sr = FileSR.FileSR(None, None)
+        sr.attach(None)
+        self.assertTrue(sr.attached)
+
+    @mock.patch("FileSR.util.makedirs", autospec=True)
+    @mock.patch("FileSR.os.chmod", autospec=True)
+    def test_attach_will_mount_if_not_already_mounted(self, mock_chmod, mock_util_makedirs):
+
+        mount_dst = "pancakes"
+        mount_src = "strawberries"
+
+        sr = FileSR.FileSR(None, None)
+
+        sr.path = mount_dst
+        sr.remotepath = mount_src
+        sr.attach(None)
+
+        self.assertTrue(sr.attached)
+
+        mount_args = self.mock_pread.call_args[0][0]
+        self.assertIn(mount_src, mount_args)
+        self.assertIn(mount_dst, mount_args)
+
+    @mock.patch("FileSR.util.makedirs", autospec=True)
+    @mock.patch("FileSR.os.chmod", autospec=True)
+    def test_attach_will_ignore_mkdir_error_if_dir_already_exists(self, mock_chmod, mock_util_makedirs):
+        sr = FileSR.FileSR(None, None)
+
+        def fake_makedirs(path, mode):
+            raise util.CommandException(errno.EEXIST)
+        mock_util_makedirs.side_effect = fake_makedirs
+        sr.path = "pancakes"
+        sr.remotepath = "strawberries"
+        sr.attach(None)
+
+        self.assertTrue(sr.attached)
+
+    @mock.patch("FileSR.util.makedirs", autospec=True)
+    def test_attach_will_rethrow_any_oserrors_on_mkdir(self, mock_util_makedirs):
+        sr = FileSR.FileSR(None, None)
+
+        def fake_makedirs(path, mode):
+            raise util.CommandException(errno.ENOMEM)
+        mock_util_makedirs.side_effect = fake_makedirs
+
+        sr.path = "pancakes"
+
+        with self.assertRaises(SR.SROSError):
+            sr.attach(None)
+
+    @mock.patch("FileSR.util.makedirs", autospec=True)
+    def test_attach_will_rethrow_any_oserrors_on_mount(self, mock_util_makedirs):
+        sr = FileSR.FileSR(None, None)
+
+        def fake_pread(cmd):
+            raise util.CommandException(errno.ENOMEM)
+        self.mock_pread.side_effect = fake_pread
+
+        sr.path = "pancakes"
+        sr.remotepath = "blueberries"
+
+        with self.assertRaises(SR.SROSError):
+            sr.attach(None)
+
+    @mock.patch("FileSR.util.makedirs", autospec=True)
+    @mock.patch("FileSR.os.chmod", autospec=True)
+    def test_attach_will_mkdir_with_closed_mode(self, mock_chmod, mock_util_makedirs):
+        dst_path = "pancakes"
+        sr = FileSR.FileSR(None, None)
+
+        sr.path = dst_path
+        sr.remotepath = "strawberries"
+        sr.attach(None)
+
+        mock_util_makedirs.assert_called_with(dst_path, mode=0o700)
+
+    @mock.patch("FileSR.util.makedirs", autospec=True)
+    @mock.patch("FileSR.os.chmod", autospec=True)
+    def test_attach_will_bind_mount_by_default(self, mock_chmod, mock_util_makedirs):
+
+        mount_dst = "pancakes"
+        mount_src = "strawberries"
+        sr = FileSR.FileSR(None, None)
+
+        sr.path = mount_dst
+        sr.remotepath = mount_src
+        sr.attach(None)
+
+        self.assertTrue(sr.attached)
+        self.assertEqual(1, len(self.mock_pread.mock_calls))
+
+        mount_args = self.mock_pread.call_args[0][0]
+        self.assertIn("--bind", mount_args)
+
+    @mock.patch("FileSR.util.makedirs", autospec=True)
+    @mock.patch("FileSR.os.chmod", autospec=True)
+    def test_attach_can_do_non_bind_mount(self, mock_chmod, mock_util_makedirs):
+
+        mount_dst = "pancakes"
+        mount_src = "strawberries"
+        sr = FileSR.FileSR(None, None)
+
+        sr.path = mount_dst
+        sr.remotepath = mount_src
+        sr.attach(None, bind=False)
+
+        self.assertTrue(sr.attached)
+
+        mount_args = self.mock_pread.call_args[0][0]
+        self.assertNotIn("--bind", mount_args)
+
+    @mock.patch("FileSR.util.makedirs", autospec=True)
+    @mock.patch("FileSR.os.chmod", autospec=True)
+    def test_attach_will_chmod_the_mount_point(self, mock_chmod, mock_util_makedirs):
+
+        mount_dst = "pancakes"
+        mount_src = "strawberries"
+        sr = FileSR.FileSR(None, None)
+
+        sr.path = mount_dst
+        sr.remotepath = mount_src
+        sr.attach(None)
+
+        mock_chmod.assert_called_with(mount_dst, mode=0o700)