From 66fe6654b74388d8a52227c1b551bbb58665ff97 Mon Sep 17 00:00:00 2001 From: Patrick Uiterwijk Date: Jan 13 2018 12:49:11 +0000 Subject: [PATCH 1/3] runroot: Allow requesting readonly extra mounts Signed-off-by: Patrick Uiterwijk --- diff --git a/plugins/builder/runroot.conf b/plugins/builder/runroot.conf index d3d222b..667c596 100644 --- a/plugins/builder/runroot.conf +++ b/plugins/builder/runroot.conf @@ -11,6 +11,13 @@ ; wildcards. ; safe_roots = /mnt/workdir/tmp +; comma-delimited list of safe readonly roots. +; Each extra_mount that is requested readonly needs to start with one of these +; prefixes or safe_roots. Only absolute paths are allowed here, no wildcards. +; Paths can be requested to be mounted as readonly by prepending the --mount argument +; with "ro:". Example: --mounts /mnt/workdir/tmp,ro:/etc/kojid/secrets +; safe_ro_roots = /etc/kojid/secrets + ; path substitutions is tuple per line, delimited by comma, order is ; important. ; Path prefixes which can be substituted for other mountpoints. diff --git a/plugins/builder/runroot.py b/plugins/builder/runroot.py index 373e880..b2a12c2 100644 --- a/plugins/builder/runroot.py +++ b/plugins/builder/runroot.py @@ -28,7 +28,7 @@ class RunRootTask(koji.tasks.BaseTaskHandler): self._read_config() return super(RunRootTask, self).__init__(*args, **kwargs) - def _get_path_params(self, path, rw=False): + def _get_path_params(self, path, rw=None): found = False for mount_data in self.config['paths']: if path.startswith(mount_data['mountpoint']): @@ -37,11 +37,18 @@ class RunRootTask(koji.tasks.BaseTaskHandler): if not found: raise koji.GenericError("bad config: missing corresponding mountpoint") options = [] + seenrx = False for o in mount_data['options'].split(','): + if o in ['ro', 'rw']: + seenrx = True if rw and o == 'ro': options.append('rw') + elif rw is False and o == 'rw': + options.append('ro') else: options.append(o) + if not seenrx: + options = ['rw' if rw else 'ro'] + options rel_path = path[len(mount_data['mountpoint']):] rel_path = rel_path[1:] if rel_path.startswith('/') else rel_path res = (os.path.join(mount_data['path'], rel_path), path, mount_data['fstype'], ','.join(options)) @@ -53,6 +60,7 @@ class RunRootTask(koji.tasks.BaseTaskHandler): self.config = { 'default_mounts': [], 'safe_roots': [], + 'safe_ro_roots': [], 'path_subs': [], 'paths': [], } @@ -61,6 +69,8 @@ class RunRootTask(koji.tasks.BaseTaskHandler): self.config['default_mounts'] = cp.get('paths', 'default_mounts').split(',') if cp.has_option('paths', 'safe_roots'): self.config['safe_roots'] = cp.get('paths', 'safe_roots').split(',') + if cp.has_option('paths', 'safe_ro_roots'): + self.config['safe_ro_roots'] = cp.get('paths', 'safe_ro_roots').split(',') if cp.has_option('paths', 'path_subs'): self.config['path_subs'] = [] for line in cp.get('paths', 'path_subs').splitlines(): @@ -86,9 +96,9 @@ class RunRootTask(koji.tasks.BaseTaskHandler): except ConfigParser.NoOptionError: raise koji.GenericError("bad config: missing options in %s section" % section_name) - for path in self.config['default_mounts'] + self.config['safe_roots'] + [x[0] for x in self.config['path_subs']]: + for path in self.config['default_mounts'] + self.config['safe_roots'] + self.config['safe_ro_roots'] + [x[0] for x in self.config['path_subs']]: if not path.startswith('/'): - raise koji.GenericError("bad config: all paths (default_mounts, safe_roots, path_subs) needs to be absolute: %s" % path) + raise koji.GenericError("bad config: all paths (default_mounts, safe_roots, safe_ro_roots, path_subs) needs to be absolute: %s" % path) def handler(self, root, arch, command, keep=False, packages=[], mounts=[], repo_id=None, skip_setarch=False, weight=None, upload_logs=None, new_chroot=False): """Create a buildroot and run a command (as root) inside of it @@ -218,21 +228,31 @@ class RunRootTask(koji.tasks.BaseTaskHandler): def do_extra_mounts(self, rootdir, mounts): mnts = [] for mount in mounts: + rw = True + # We copy the list, since we might be modifying it + safe_roots = self.config['safe_roots'][:] + if mount.startswith('ro:'): + mount = mount[len('ro:'):] + safe_roots += self.config['safe_ro_roots'] + rw = False mount = os.path.normpath(mount) - for safe_root in self.config['safe_roots']: + for safe_root in safe_roots: if mount.startswith(safe_root): break else: #no match - raise koji.GenericError("read-write mount point is not safe: %s" % mount) + if rw: + raise koji.GenericError("read-write mount point is not safe: %s" % mount) + else: + raise koji.GenericError("read-only mount point is not safe: %s" % mount) #normpath should have removed any .. dirs, but just in case... if mount.find('/../') != -1: - raise koji.GenericError("read-write mount point is not safe: %s" % mount) + raise koji.GenericError("requested mount point is not safe: %s" % mount) for rep, sub in self.config['path_subs']: mount = mount.replace(rep, sub) - mnts.append(self._get_path_params(mount, rw=True)) + mnts.append(self._get_path_params(mount, rw=rw)) self.do_mounts(rootdir, mnts) def do_mounts(self, rootdir, mounts): diff --git a/plugins/cli/runroot.py b/plugins/cli/runroot.py index f8d4b50..5ac7c99 100644 --- a/plugins/cli/runroot.py +++ b/plugins/cli/runroot.py @@ -14,7 +14,7 @@ def handle_runroot(options, session, args): parser = OptionParser(usage=usage) parser.disable_interspersed_args() parser.add_option("-p", "--package", action="append", default=[], help=_("make sure this package is in the chroot")) - parser.add_option("-m", "--mount", action="append", default=[], help=_("mount this directory read-write in the chroot")) + parser.add_option("-m", "--mount", action="append", default=[], help=_("mount this directory read-write in the chroot, prefix with ro: to request readonly mount")) parser.add_option("--skip-setarch", action="store_true", default=False, help=_("bypass normal setarch in the chroot")) parser.add_option("-w", "--weight", type='int', help=_("set task weight")) diff --git a/tests/test_plugins/test_runroot_builder.py b/tests/test_plugins/test_runroot_builder.py index 21ffc12..9d9a882 100644 --- a/tests/test_plugins/test_runroot_builder.py +++ b/tests/test_plugins/test_runroot_builder.py @@ -17,6 +17,7 @@ CONFIG1 = { 'paths': { 'default_mounts': '/mnt/archive,/mnt/workdir', 'safe_roots': '/mnt/workdir/tmp', + 'safe_ro_roots': '/mnt/workdir/ro/tmp', 'path_subs': '/mnt/archive/prehistory/,/mnt/prehistoric_disk/archive/prehistory', }, @@ -25,6 +26,12 @@ CONFIG1 = { 'path': 'archive.org:/vol/archive', 'fstype': 'nfs', 'options': 'ro,hard,intr,nosuid,nodev,noatime,tcp', + }, + 'path1': { + 'mountpoint': '/mnt/workdir', + 'path': 'archive.org:/vol/workdir', + 'fstype': 'nfs', + 'options': 'rw,hard,intr,nosuid,nodev,noatime,tcp', }} @@ -32,6 +39,7 @@ CONFIG2 = { 'paths': { 'default_mounts': '/mnt/archive,/mnt/workdir', 'safe_roots': '/mnt/workdir/tmp', + 'safe_ro_roots': '/mnt/workdir/ro/tmp', 'path_subs': '\n' '/mnt/archive/prehistory/,/mnt/prehistoric_disk/archive/prehistory\n' @@ -109,7 +117,7 @@ class TestRunrootConfig(unittest.TestCase): with self.assertRaises(koji.GenericError) as cm: runroot.RunRootTask(123, 'runroot', {}, session, options) self.assertEqual(cm.exception.message, - "bad config: all paths (default_mounts, safe_roots, path_subs) needs to be absolute: ") + "bad config: all paths (default_mounts, safe_roots, safe_ro_roots, path_subs) needs to be absolute: ") @mock.patch('ConfigParser.SafeConfigParser') def test_valid_config(self, safe_config_parser): @@ -178,6 +186,30 @@ class TestMounts(unittest.TestCase): self.assertEqual(self.t._get_path_params('/mnt/archive', 'rw'), ('archive.org:/vol/archive/', '/mnt/archive', 'nfs', 'rw,hard,intr,nosuid,nodev,noatime,tcp')) + # ro volume, no rw flag + self.assertEqual(self.t._get_path_params('/mnt/archive'), + ('archive.org:/vol/archive/', '/mnt/archive', 'nfs', 'ro,hard,intr,nosuid,nodev,noatime,tcp')) + + # ro volume, rw True + self.assertEqual(self.t._get_path_params('/mnt/archive', True), + ('archive.org:/vol/archive/', '/mnt/archive', 'nfs', 'rw,hard,intr,nosuid,nodev,noatime,tcp')) + + # ro volume, rw False + self.assertEqual(self.t._get_path_params('/mnt/archive', False), + ('archive.org:/vol/archive/', '/mnt/archive', 'nfs', 'ro,hard,intr,nosuid,nodev,noatime,tcp')) + + # rw volume, no rw flag + self.assertEqual(self.t._get_path_params('/mnt/workdir'), + ('archive.org:/vol/workdir/', '/mnt/workdir', 'nfs', 'rw,hard,intr,nosuid,nodev,noatime,tcp')) + + # rw volume, rw True + self.assertEqual(self.t._get_path_params('/mnt/workdir', True), + ('archive.org:/vol/workdir/', '/mnt/workdir', 'nfs', 'rw,hard,intr,nosuid,nodev,noatime,tcp')) + + # rw volume, rw False + self.assertEqual(self.t._get_path_params('/mnt/workdir', False), + ('archive.org:/vol/workdir/', '/mnt/workdir', 'nfs', 'ro,hard,intr,nosuid,nodev,noatime,tcp')) + @mock.patch('os.path.isdir') @mock.patch('runroot.open') @mock.patch('runroot.log_output') @@ -263,8 +295,30 @@ class TestMounts(unittest.TestCase): # safe mount self.t.do_mounts.reset_mock() + self.t._get_path_params.reset_mock() self.t.do_extra_mounts('rootdir', ['/mnt/workdir/tmp/xyz']) self.t.do_mounts.assert_called_once_with('rootdir', ['path_params']) + self.t._get_path_params.assert_called_once_with('/mnt/workdir/tmp/xyz', rw=True) + + # safe RO mount + self.t.do_mounts.reset_mock() + self.t._get_path_params.reset_mock() + self.t.do_extra_mounts('rootdir', ['ro:/mnt/workdir/ro/tmp/xyz']) + self.t.do_mounts.assert_called_once_with('rootdir', ['path_params']) + self.t._get_path_params.assert_called_once_with('/mnt/workdir/ro/tmp/xyz', rw=False) + + # RO mount safe for RW + self.t.do_mounts.reset_mock() + self.t._get_path_params.reset_mock() + self.t.do_extra_mounts('rootdir', ['ro:/mnt/workdir/tmp/xyz']) + self.t.do_mounts.assert_called_once_with('rootdir', ['path_params']) + self.t._get_path_params.assert_called_once_with('/mnt/workdir/tmp/xyz', rw=False) + + # RO-safe mounted as RW + self.t.do_mounts.reset_mock() + with self.assertRaises(koji.GenericError): + self.t.do_extra_mounts('rootdir', ['/mnt/workdir/ro/tmp/xyz']) + self.t.do_mounts.assert_not_called() # unsafe mount self.t.do_mounts.reset_mock() @@ -272,6 +326,12 @@ class TestMounts(unittest.TestCase): self.t.do_extra_mounts('rootdir', ['unsafe']) self.t.do_mounts.assert_not_called() + # unsafe RO mount + self.t.do_mounts.reset_mock() + with self.assertRaises(koji.GenericError): + self.t.do_extra_mounts('rootdir', ['ro:unsafe']) + self.t.do_mounts.assert_not_called() + # hackish mount self.t.do_mounts.reset_mock() with self.assertRaises(koji.GenericError): From 5e3555e04cc752466bb0afa2d86c1e928c92d476 Mon Sep 17 00:00:00 2001 From: Patrick Uiterwijk Date: Jan 15 2018 10:37:23 +0000 Subject: [PATCH 2/3] runroot: Allow requiring permissions for mount paths Signed-off-by: Patrick Uiterwijk --- diff --git a/plugins/builder/runroot.conf b/plugins/builder/runroot.conf index 667c596..70d2ad8 100644 --- a/plugins/builder/runroot.conf +++ b/plugins/builder/runroot.conf @@ -30,3 +30,4 @@ ; path = archive.org:/vol/archive ; fstype = nfs ; options = ro,hard,intr,nosuid,nodev,noatime,tcp +; permission = myperm diff --git a/plugins/builder/runroot.py b/plugins/builder/runroot.py index b2a12c2..a2746f8 100644 --- a/plugins/builder/runroot.py +++ b/plugins/builder/runroot.py @@ -36,6 +36,10 @@ class RunRootTask(koji.tasks.BaseTaskHandler): break if not found: raise koji.GenericError("bad config: missing corresponding mountpoint") + if mount_data.get('permission'): + perm = mount_data['permission'] + if perm not in self.task_owner_perms and 'admin' not in self.task_owner_perms: + raise koji.GenericError('permission %s is required to mount %s' % (perm, path)) options = [] seenrx = False for o in mount_data['options'].split(','): @@ -87,11 +91,15 @@ class RunRootTask(koji.tasks.BaseTaskHandler): path_sections = [p for p in cp.sections() if re.match('path\d+', p)] for section_name in sorted(path_sections, key=lambda x: int(x[4:])): try: + perm = None + if cp.has_option(section_name, 'permission'): + perm = cp.get(section_name, 'permission') self.config['paths'].append({ 'mountpoint': cp.get(section_name, 'mountpoint'), 'path': cp.get(section_name, 'path'), 'fstype': cp.get(section_name, 'fstype'), 'options': cp.get(section_name, 'options'), + 'permission': perm, }) except ConfigParser.NoOptionError: raise koji.GenericError("bad config: missing options in %s section" % section_name) @@ -117,6 +125,8 @@ class RunRootTask(koji.tasks.BaseTaskHandler): archiving on hub. It always consists of /tmp/runroot.log, but can be used for additional logs (pungi.log, etc.) """ + self.task_info = self.session.getTaskInfo(self.id) + self.task_owner_perms = self.session.getUserPerms(self.task_info['owner']) if weight is not None: weight = max(weight, 0.5) self.session.host.setTaskWeight(self.id, weight) diff --git a/tests/test_plugins/test_runroot_builder.py b/tests/test_plugins/test_runroot_builder.py index 9d9a882..f028c34 100644 --- a/tests/test_plugins/test_runroot_builder.py +++ b/tests/test_plugins/test_runroot_builder.py @@ -32,6 +32,13 @@ CONFIG1 = { 'path': 'archive.org:/vol/workdir', 'fstype': 'nfs', 'options': 'rw,hard,intr,nosuid,nodev,noatime,tcp', + }, + 'path2': { + 'mountpoint': '/mnt/secrets', + 'path': '/vol/secrets', + 'fstype': 'nfs', + 'options': 'ro,hard,intr,nosuid,nodev,noatime,tcp', + 'permission': 'myperm', }} @@ -154,6 +161,10 @@ class TestRunrootConfig(unittest.TestCase): # resulting processed config should be the same self.assertEqual(task1.config, task2.config) paths = list([CONFIG2[k] for k in ('path0', 'path1', 'path2')]) + for path in paths: + if not 'permission' in path: + # If there's no permission configured, we default to None + path['permission'] = None self.assertEqual(task2.config['paths'], paths) @mock.patch('ConfigParser.SafeConfigParser') @@ -210,6 +221,26 @@ class TestMounts(unittest.TestCase): self.assertEqual(self.t._get_path_params('/mnt/workdir', False), ('archive.org:/vol/workdir/', '/mnt/workdir', 'nfs', 'ro,hard,intr,nosuid,nodev,noatime,tcp')) + # item with permission: no permission + self.t.task_owner_perms = [] + with self.assertRaises(koji.GenericError): + self.t._get_path_params('/mnt/secrets') + + # item with permission: missing permission + self.t.task_owner_perms = ['someperm', 'otherperm'] + with self.assertRaises(koji.GenericError): + self.t._get_path_params('/mnt/secrets') + + # item with permission: normal permission + self.t.task_owner_perms = ['myperm'] + self.assertEqual(self.t._get_path_params('/mnt/secrets'), + ('/vol/secrets/', '/mnt/secrets', 'nfs', 'ro,hard,intr,nosuid,nodev,noatime,tcp')) + + # item with permission: admin permission + self.t.task_owner_perms = ['admin'] + self.assertEqual(self.t._get_path_params('/mnt/secrets'), + ('/vol/secrets/', '/mnt/secrets', 'nfs', 'ro,hard,intr,nosuid,nodev,noatime,tcp')) + @mock.patch('os.path.isdir') @mock.patch('runroot.open') @mock.patch('runroot.log_output') @@ -401,6 +432,7 @@ class TestHandler(unittest.TestCase): @mock.patch('os.system') def test_handler_simple(self, os_system, platform_uname): platform_uname.return_value = ('system', 'node', 'release', 'version', 'machine', 'arch') + self.session.getUserPerms.return_value = ['someperm', 'otherperm'] self.session.getBuildConfig.return_value = { 'id': 456, 'name': 'tag_name', @@ -445,3 +477,4 @@ class TestHandler(unittest.TestCase): mock.call('/rootdir/log_1'), mock.call('/rootdir/log_2'), ]) + self.assertEqual(self.t.task_owner_perms, ['someperm', 'otherperm']) From c9ccd9a4cbb69c80d33b60f65938a7e0cec486c7 Mon Sep 17 00:00:00 2001 From: Patrick Uiterwijk Date: Jan 15 2018 10:37:26 +0000 Subject: [PATCH 3/3] Be strict regarding the _get_path_params type Signed-off-by: Patrick Uiterwijk --- diff --git a/plugins/builder/runroot.py b/plugins/builder/runroot.py index a2746f8..7051e24 100644 --- a/plugins/builder/runroot.py +++ b/plugins/builder/runroot.py @@ -29,6 +29,8 @@ class RunRootTask(koji.tasks.BaseTaskHandler): return super(RunRootTask, self).__init__(*args, **kwargs) def _get_path_params(self, path, rw=None): + if rw not in [None, True, False]: + raise ValueError('Invalid RW flag provided to get_path_params') found = False for mount_data in self.config['paths']: if path.startswith(mount_data['mountpoint']): @@ -45,7 +47,7 @@ class RunRootTask(koji.tasks.BaseTaskHandler): for o in mount_data['options'].split(','): if o in ['ro', 'rw']: seenrx = True - if rw and o == 'ro': + if rw is True and o == 'ro': options.append('rw') elif rw is False and o == 'rw': options.append('ro') diff --git a/tests/test_plugins/test_runroot_builder.py b/tests/test_plugins/test_runroot_builder.py index f028c34..b5adcbf 100644 --- a/tests/test_plugins/test_runroot_builder.py +++ b/tests/test_plugins/test_runroot_builder.py @@ -37,7 +37,7 @@ CONFIG1 = { 'mountpoint': '/mnt/secrets', 'path': '/vol/secrets', 'fstype': 'nfs', - 'options': 'ro,hard,intr,nosuid,nodev,noatime,tcp', + 'options': 'hard,intr,nosuid,nodev,noatime,tcp', 'permission': 'myperm', }} @@ -194,7 +194,7 @@ class TestMounts(unittest.TestCase): self.t._get_path_params('nonexistent_dir') # valid item - self.assertEqual(self.t._get_path_params('/mnt/archive', 'rw'), + self.assertEqual(self.t._get_path_params('/mnt/archive', True), ('archive.org:/vol/archive/', '/mnt/archive', 'nfs', 'rw,hard,intr,nosuid,nodev,noatime,tcp')) # ro volume, no rw flag @@ -241,6 +241,22 @@ class TestMounts(unittest.TestCase): self.assertEqual(self.t._get_path_params('/mnt/secrets'), ('/vol/secrets/', '/mnt/secrets', 'nfs', 'ro,hard,intr,nosuid,nodev,noatime,tcp')) + # norx volume, no rw flag + self.assertEqual(self.t._get_path_params('/mnt/secrets'), + ('/vol/secrets/', '/mnt/secrets', 'nfs', 'ro,hard,intr,nosuid,nodev,noatime,tcp')) + + # norx volume, rw True + self.assertEqual(self.t._get_path_params('/mnt/secrets', True), + ('/vol/secrets/', '/mnt/secrets', 'nfs', 'rw,hard,intr,nosuid,nodev,noatime,tcp')) + + # norx volume, rw False + self.assertEqual(self.t._get_path_params('/mnt/secrets', False), + ('/vol/secrets/', '/mnt/secrets', 'nfs', 'ro,hard,intr,nosuid,nodev,noatime,tcp')) + + # invalid rw value + with self.assertRaises(ValueError): + self.t._get_path_params('/mnt/secrets', 'rw') + @mock.patch('os.path.isdir') @mock.patch('runroot.open') @mock.patch('runroot.log_output')