From dbf77fba1061cb4e93b3db5f8117d6ccc689f702 Mon Sep 17 00:00:00 2001 From: Angus Lees Date: Mon, 8 Feb 2016 12:27:40 +1100 Subject: [PATCH] Trivial rootwrap -> privsep replacement This change replaces all uses of rootwrap with a trivial privsep-based equivalent. This replacement simply executes commands as the privsep user *without any additional checks*. There are 2 reasons why this is a reasonable thing to do: 1. We don't have a good workflow for merging rootwrap filter changes into parent projects (nova/cinder) for a loosely-coupled library like os-brick. 2. The previous situation was also insecure. The os-brick.filters rootwrap config permitted commands like "dd" and "cp" with any arguments, as root. This would have posed only a mild inconvenience to an attacker. With privsep we can at least (in principle) limit the commands to the privsep uid/gid and Linux capabilities (CAP_SYS_ADMIN by default with this change). This change addresses the urgency of (1). Later refactors will take greater advantage of privsep to address (2). Change-Id: I0af542eba97d2f89b1c283bf1e1e985d9690f5de Depends-On: I90dc41bc77993bd83b80c92286e015e14f290b45 # nova: nova.conf: Set privsep_rootwrap.helper_command Depends-On: I4e333e73ddfd45c045b9d32dac1506fc25858c4d # nova: Add os-brick rootwrap filter for privsep Depends-On: I8a0b1728cc66c4861f69623b1b16b1f759b57b25 # cinder: cinder.conf: Set privsep_rootwrap.helper_command Depends-On: I3b2e337321875cf4abc0ab9b44fe17cf9327d88b # cinder: Add os-brick rootwrap filter for privsep Depends-On: I4299c2fc059807610f83e12a2d470e020930c64c # privsep: Switch to msgpack for serialization Depends-On: Ied1ef4fc945e18516b39d1f20d58425cb633dc74 # requirements: require oslo.privsep>=1.5.0 for msgpack fix --- etc/os-brick/rootwrap.d/os-brick.filters | 105 +-------------------- os_brick/executor.py | 11 ++- os_brick/initiator/connector.py | 67 ++++--------- os_brick/initiator/linuxfc.py | 10 -- os_brick/initiator/linuxscsi.py | 7 +- os_brick/local_dev/lvm.py | 27 +++--- os_brick/privileged/__init__.py | 23 +++++ os_brick/privileged/rootwrap.py | 82 ++++++++++++++++ os_brick/remotefs/remotefs.py | 11 ++- os_brick/tests/initiator/test_connector.py | 9 +- os_brick/tests/local_dev/test_brick_lvm.py | 19 ++-- os_brick/tests/privileged/__init__.py | 0 os_brick/tests/privileged/test_rootwrap.py | 59 ++++++++++++ os_brick/tests/remotefs/test_remotefs.py | 22 ++--- requirements.txt | 1 + 15 files changed, 243 insertions(+), 210 deletions(-) create mode 100644 os_brick/privileged/__init__.py create mode 100644 os_brick/privileged/rootwrap.py create mode 100644 os_brick/tests/privileged/__init__.py create mode 100644 os_brick/tests/privileged/test_rootwrap.py diff --git a/etc/os-brick/rootwrap.d/os-brick.filters b/etc/os-brick/rootwrap.d/os-brick.filters index 4c5507f21..67f9aedad 100644 --- a/etc/os-brick/rootwrap.d/os-brick.filters +++ b/etc/os-brick/rootwrap.d/os-brick.filters @@ -2,104 +2,7 @@ # This file should be owned by (and only-writeable by) the root user [Filters] -# remotefs/remotefs.py: 'mount', '-t', 'sofs' ... -mount: CommandFilter, mount, root - -# initiator/linuxscsi.py: 'blockdev', '--flushbufs', device -blockdev: RegExpFilter, blockdev, root, blockdev, (--getsize64|--flushbufs), /dev/.* - -# initiator/linuxscsi.py: 'tee', canonpath -tee: CommandFilter, tee, root - -# remotefs/remotefs.py: 'mkdir', canonpath -mkdir: CommandFilter, mkdir, root - -# remotefs/remotefs.py: 'chown', '-R', 'root' -chown: RegExpFilter, chown, root, chown root:root /etc/pstorage/clusters/(?!.*/\.\.).* - -# initiator/connector.py: 'ip', 'addr', 'list' -ip: CommandFilter, ip, root - -# initiator/connector.py: 'dd', if=%(path)s % ("path": path} -dd: CommandFilter, dd, root - -# initiator/connector.py: 'iscsiadm', '-m', ... -iscsiadm: CommandFilter, iscsiadm, root - -# initiator/connector.py: 'aoe-revalidate', aoedev -# initiator/connector.py: 'aoe-discover' -# initiator/connector.py: 'aoe-flush' -aoe-revalidate: CommandFilter, aoe-revalidate, root -aoe-discover: CommandFilter, aoe-discover, root -aoe-flush: CommandFilter, aoe-flush, root - -# initiator/connector.py: -read_initiator: ReadFileFilter, /etc/iscsi/initiatorname.iscsi - -# initiator/connector.py: 'multipath', '-ll' -# initiator/linuxscsi.py: 'multipath', '-ll' -multipath: CommandFilter, multipath, root - -# initiator/connector.py: 'multipathd', 'show', 'status' -multipathd: CommandFilter, multipathd, root - -# initiator/linuxfc.py: 'systool', '-c', 'fc_host', '-v' -systool: CommandFilter, systool, root - -# initiator/linuxscsi.py:: 'sg_scan', device -sg_scan: CommandFilter, sg_scan, root - -# remotefs/remotefs.py: 'cp', '-f', tmp_bs_path -cp: CommandFilter, cp, root - -# initiator/connector.py: -drv_cfg: CommandFilter, /opt/emc/scaleio/sdc/bin/drv_cfg, root, /opt/emc/scaleio/sdc/bin/drv_cfg, --query_guid - -# initiator/connector.py -sds_cli: CommandFilter, /usr/local/bin/sds/sds_cli, root - -# initiator/connector.py -drbdadm: CommandFilter, drbdadm, root - -# initiator/connector.py: 'vgc-cluster', 'domain-list', '-l' -# initiator/connector.py: 'vgc-cluster', 'space-set-apphosts', '-n'... -vgc-cluster: CommandFilter, vgc-cluster, root - -# initiator/linuxscsi.py -scsi_id: CommandFilter, /lib/udev/scsi_id, root - -# local_dev lvm related commands - -# LVM related show commands -pvs: EnvFilter, env, root, LC_ALL=C, pvs -vgs: EnvFilter, env, root, LC_ALL=C, vgs -lvs: EnvFilter, env, root, LC_ALL=C, lvs -lvdisplay: EnvFilter, env, root, LC_ALL=C, lvdisplay - -# local_dev/lvm.py: 'vgcreate', vg_name, pv_list -vgcreate: CommandFilter, vgcreate, root - -# local_dev/lvm.py: 'lvcreate', '-L', sizestr, '-n', volume_name,.. -# local_dev/lvm.py: 'lvcreate', '-L', ... -lvcreate: EnvFilter, env, root, LC_ALL=C, lvcreate -lvcreate_lvmconf: EnvFilter, env, root, LVM_SYSTEM_DIR=, LC_ALL=C, lvcreate - -# local_dev/lvm.py: 'lvextend', '-L' '%(new_size)s', '%(lv_name)s' ... -# local_dev/lvm.py: 'lvextend', '-L' '%(new_size)s', '%(thin_pool)s' ... -lvextend: EnvFilter, env, root, LC_ALL=C, lvextend -lvextend_lvmconf: EnvFilter, env, root, LVM_SYSTEM_DIR=, LC_ALL=C, lvextend - -# local_dev/lvm.py: 'lvremove', '-f', %s/%s % ... -lvremove: CommandFilter, lvremove, root - -# local_dev/lvm.py: 'lvrename', '%(vg)s', '%(orig)s' '(new)s'... -lvrename: CommandFilter, lvrename, root - -# local_dev/lvm.py: 'lvchange -a y -K ' -lvchange: CommandFilter, lvchange, root - -# local_dev/lvm.py: 'lvconvert', '--merge', snapshot_name -lvconvert: CommandFilter, lvconvert, root - -# local_dev/lvm.py: 'udevadm', 'settle' -udevadm: CommandFilter, udevadm, root +# privileged/__init__.py: priv_context.PrivContext(default) +# This line ties the superuser privs with the config files, context name, +# and (implicitly) the actual python code invoked. +privsep-rootwrap: RegExpFilter, privsep-helper, root, privsep-helper, --config-file, /etc/(?!\.\.).*, --privsep_context, os_brick.privileged.default, --privsep_sock_path, /tmp/.* diff --git a/os_brick/executor.py b/os_brick/executor.py index fe82b8826..17701a403 100644 --- a/os_brick/executor.py +++ b/os_brick/executor.py @@ -20,10 +20,19 @@ from oslo_concurrency import processutils as putils +from os_brick.privileged import rootwrap as priv_rootwrap + class Executor(object): - def __init__(self, root_helper, execute=putils.execute, + def __init__(self, root_helper, execute=None, *args, **kwargs): + # For backwards compatibility, `putils.execute` is interpreted + # as a sentinel to mean "I want the os-brick default" :-/ + # This can be burnt as soon as we update all the callsites (in + # nova+cinder) to the new default - and then we shall never + # speak of it again. + if execute is None or execute is putils.execute: + execute = priv_rootwrap.execute self.set_execute(execute) self.set_root_helper(root_helper) diff --git a/os_brick/initiator/connector.py b/os_brick/initiator/connector.py index caae03f86..ba1f63c1a 100644 --- a/os_brick/initiator/connector.py +++ b/os_brick/initiator/connector.py @@ -54,6 +54,7 @@ from os_brick.initiator import linuxfc from os_brick.initiator import linuxrbd from os_brick.initiator import linuxscsi from os_brick.initiator import linuxsheepdog +from os_brick.privileged import rootwrap as priv_rootwrap from os_brick.remotefs import remotefs from os_brick.i18n import _, _LE, _LI, _LW @@ -86,8 +87,8 @@ SHEEPDOG = "SHEEPDOG" def _check_multipathd_running(root_helper, enforce_multipath): try: - putils.execute('multipathd', 'show', 'status', - run_as_root=True, root_helper=root_helper) + priv_rootwrap.execute('multipathd', 'show', 'status', + run_as_root=True, root_helper=root_helper) except putils.ProcessExecutionError as err: LOG.error(_LE('multipathd is not running: exit code %(err)s'), {'err': err.exit_code}) @@ -148,8 +149,7 @@ def get_connector_properties(root_helper, my_ip, multipath, enforce_multipath, @six.add_metaclass(abc.ABCMeta) class InitiatorConnector(executor.Executor): - def __init__(self, root_helper, driver=None, - execute=putils.execute, + def __init__(self, root_helper, driver=None, execute=None, device_scan_attempts=DEVICE_SCAN_ATTEMPTS_DEFAULT, *args, **kwargs): super(InitiatorConnector, self).__init__(root_helper, execute=execute, @@ -167,7 +167,7 @@ class InitiatorConnector(executor.Executor): @staticmethod def factory(protocol, root_helper, driver=None, - execute=putils.execute, use_multipath=False, + use_multipath=False, device_scan_attempts=DEVICE_SCAN_ATTEMPTS_DEFAULT, arch=None, *args, **kwargs): @@ -188,7 +188,6 @@ class InitiatorConnector(executor.Executor): kwargs.update({'transport': 'iser'}) return ISCSIConnector(root_helper=root_helper, driver=driver, - execute=execute, use_multipath=use_multipath, device_scan_attempts=device_scan_attempts, *args, **kwargs) @@ -197,7 +196,6 @@ class InitiatorConnector(executor.Executor): return FibreChannelConnectorS390X( root_helper=root_helper, driver=driver, - execute=execute, use_multipath=use_multipath, device_scan_attempts=device_scan_attempts, *args, **kwargs) @@ -205,72 +203,61 @@ class InitiatorConnector(executor.Executor): return FibreChannelConnector( root_helper=root_helper, driver=driver, - execute=execute, use_multipath=use_multipath, device_scan_attempts=device_scan_attempts, *args, **kwargs) elif protocol == AOE: return AoEConnector(root_helper=root_helper, driver=driver, - execute=execute, device_scan_attempts=device_scan_attempts, *args, **kwargs) elif protocol in (NFS, GLUSTERFS, SCALITY, QUOBYTE, VZSTORAGE): return RemoteFsConnector(mount_type=protocol.lower(), root_helper=root_helper, driver=driver, - execute=execute, device_scan_attempts=device_scan_attempts, *args, **kwargs) elif protocol == DRBD: return DRBDConnector(root_helper=root_helper, driver=driver, - execute=execute, *args, **kwargs) elif protocol == LOCAL: return LocalConnector(root_helper=root_helper, driver=driver, - execute=execute, device_scan_attempts=device_scan_attempts, *args, **kwargs) elif protocol == HUAWEISDSHYPERVISOR: return HuaweiStorHyperConnector( root_helper=root_helper, driver=driver, - execute=execute, device_scan_attempts=device_scan_attempts, *args, **kwargs) elif protocol == HGST: return HGSTConnector(root_helper=root_helper, driver=driver, - execute=execute, device_scan_attempts=device_scan_attempts, *args, **kwargs) elif protocol == RBD: return RBDConnector(root_helper=root_helper, driver=driver, - execute=execute, device_scan_attempts=device_scan_attempts, *args, **kwargs) elif protocol == SCALEIO: return ScaleIOConnector( root_helper=root_helper, driver=driver, - execute=execute, device_scan_attempts=device_scan_attempts, *args, **kwargs) elif protocol == DISCO: return DISCOConnector( root_helper=root_helper, driver=driver, - execute=execute, device_scan_attempts=device_scan_attempts, *args, **kwargs ) elif protocol == SHEEPDOG: return SheepdogConnector(root_helper=root_helper, driver=driver, - execute=execute, device_scan_attempts=device_scan_attempts, *args, **kwargs) else: @@ -515,7 +502,7 @@ class ISCSIConnector(InitiatorConnector): 'cxgb4i', 'qla4xxx', 'ocs', 'iser'] def __init__(self, root_helper, driver=None, - execute=putils.execute, use_multipath=False, + execute=None, use_multipath=False, device_scan_attempts=DEVICE_SCAN_ATTEMPTS_DEFAULT, transport='default', *args, **kwargs): self._linuxscsi = linuxscsi.LinuxSCSI(root_helper, execute) @@ -1308,7 +1295,7 @@ class FibreChannelConnector(InitiatorConnector): """Connector class to attach/detach Fibre Channel volumes.""" def __init__(self, root_helper, driver=None, - execute=putils.execute, use_multipath=False, + execute=None, use_multipath=False, device_scan_attempts=DEVICE_SCAN_ATTEMPTS_DEFAULT, *args, **kwargs): self._linuxscsi = linuxscsi.LinuxSCSI(root_helper, execute) @@ -1559,7 +1546,7 @@ class FibreChannelConnectorS390X(FibreChannelConnector): """Connector class to attach/detach Fibre Channel volumes on S390X arch.""" def __init__(self, root_helper, driver=None, - execute=putils.execute, use_multipath=False, + execute=None, use_multipath=False, device_scan_attempts=DEVICE_SCAN_ATTEMPTS_DEFAULT, *args, **kwargs): super(FibreChannelConnectorS390X, self).__init__( @@ -1621,13 +1608,11 @@ class FibreChannelConnectorS390X(FibreChannelConnector): class AoEConnector(InitiatorConnector): """Connector class to attach/detach AoE volumes.""" def __init__(self, root_helper, driver=None, - execute=putils.execute, device_scan_attempts=DEVICE_SCAN_ATTEMPTS_DEFAULT, *args, **kwargs): super(AoEConnector, self).__init__( root_helper, driver=driver, - execute=execute, device_scan_attempts=device_scan_attempts, *args, **kwargs) @@ -1764,7 +1749,7 @@ class RemoteFsConnector(InitiatorConnector): """Connector class to attach/detach NFS and GlusterFS volumes.""" def __init__(self, mount_type, root_helper, driver=None, - execute=putils.execute, + execute=None, device_scan_attempts=DEVICE_SCAN_ATTEMPTS_DEFAULT, *args, **kwargs): kwargs = kwargs or {} @@ -1852,13 +1837,11 @@ class RemoteFsConnector(InitiatorConnector): class RBDConnector(InitiatorConnector): """"Connector class to attach/detach RBD volumes.""" - def __init__(self, root_helper, driver=None, - execute=putils.execute, use_multipath=False, + def __init__(self, root_helper, driver=None, use_multipath=False, device_scan_attempts=DEVICE_SCAN_ATTEMPTS_DEFAULT, *args, **kwargs): super(RBDConnector, self).__init__(root_helper, driver=driver, - execute=execute, device_scan_attempts= device_scan_attempts, *args, **kwargs) @@ -1944,10 +1927,10 @@ class RBDConnector(InitiatorConnector): class LocalConnector(InitiatorConnector): """"Connector class to attach/detach File System backed volumes.""" - def __init__(self, root_helper, driver=None, execute=putils.execute, + def __init__(self, root_helper, driver=None, *args, **kwargs): super(LocalConnector, self).__init__(root_helper, driver=driver, - execute=execute, *args, **kwargs) + *args, **kwargs) def get_volume_paths(self, connection_properties): path = connection_properties['device_path'] @@ -1998,15 +1981,6 @@ class LocalConnector(InitiatorConnector): class DRBDConnector(InitiatorConnector): """"Connector class to attach/detach DRBD resources.""" - def __init__(self, root_helper, driver=None, - execute=putils.execute, *args, **kwargs): - - super(DRBDConnector, self).__init__(root_helper, driver=driver, - execute=execute, *args, **kwargs) - - self._execute = execute - self._root_helper = root_helper - def check_valid_device(self, path, run_as_root=True): """Verify an existing volume.""" # TODO(linbit): check via drbdsetup first, to avoid blocking/hanging @@ -2085,7 +2059,7 @@ class HuaweiStorHyperConnector(InitiatorConnector): not_mount_node_code = 50155007 iscliexist = True - def __init__(self, root_helper, driver=None, execute=putils.execute, + def __init__(self, root_helper, driver=None, *args, **kwargs): self.cli_path = os.getenv('HUAWEISDSHYPERVISORCLI_PATH') if not self.cli_path: @@ -2098,7 +2072,6 @@ class HuaweiStorHyperConnector(InitiatorConnector): 'HuaweiStorHyperConnector init failed.')) super(HuaweiStorHyperConnector, self).__init__(root_helper, driver=driver, - execute=execute, *args, **kwargs) def get_search_path(self): @@ -2240,11 +2213,9 @@ class HGSTConnector(InitiatorConnector): VGCCLUSTER = 'vgc-cluster' def __init__(self, root_helper, driver=None, - execute=putils.execute, device_scan_attempts=DEVICE_SCAN_ATTEMPTS_DEFAULT, *args, **kwargs): super(HGSTConnector, self).__init__(root_helper, driver=driver, - execute=execute, device_scan_attempts= device_scan_attempts, *args, **kwargs) @@ -2389,15 +2360,14 @@ class ScaleIOConnector(InitiatorConnector): OK_STATUS_CODE = 200 VOLUME_NOT_MAPPED_ERROR = 84 VOLUME_ALREADY_MAPPED_ERROR = 81 - GET_GUID_CMD = ['drv_cfg', '--query_guid'] + GET_GUID_CMD = ['/opt/emc/scaleio/sdc/bin/drv_cfg', '--query_guid'] - def __init__(self, root_helper, driver=None, execute=putils.execute, + def __init__(self, root_helper, driver=None, device_scan_attempts=DEVICE_SCAN_ATTEMPTS_DEFAULT, *args, **kwargs): super(ScaleIOConnector, self).__init__( root_helper, driver=driver, - execute=execute, device_scan_attempts=device_scan_attempts, *args, **kwargs ) @@ -2839,14 +2809,13 @@ class DISCOConnector(InitiatorConnector): DISCO_PREFIX = 'dms' - def __init__(self, root_helper, driver=None, execute=putils.execute, + def __init__(self, root_helper, driver=None, device_scan_attempts=DEVICE_SCAN_ATTEMPTS_DEFAULT, *args, **kwargs): """Init DISCO connector.""" super(DISCOConnector, self).__init__( root_helper, driver=driver, - execute=execute, device_scan_attempts=device_scan_attempts, *args, **kwargs ) @@ -3006,13 +2975,11 @@ class DISCOConnector(InitiatorConnector): class SheepdogConnector(InitiatorConnector): """"Connector class to attach/detach sheepdog volumes.""" - def __init__(self, root_helper, driver=None, - execute=putils.execute, use_multipath=False, + def __init__(self, root_helper, driver=None, use_multipath=False, device_scan_attempts=DEVICE_SCAN_ATTEMPTS_DEFAULT, *args, **kwargs): super(SheepdogConnector, self).__init__(root_helper, driver=driver, - execute=execute, device_scan_attempts= device_scan_attempts, *args, **kwargs) diff --git a/os_brick/initiator/linuxfc.py b/os_brick/initiator/linuxfc.py index f81a655ea..ca256e9f3 100644 --- a/os_brick/initiator/linuxfc.py +++ b/os_brick/initiator/linuxfc.py @@ -27,11 +27,6 @@ LOG = logging.getLogger(__name__) class LinuxFibreChannel(linuxscsi.LinuxSCSI): - def __init__(self, root_helper, execute=putils.execute, - *args, **kwargs): - super(LinuxFibreChannel, self).__init__(root_helper, execute, - *args, **kwargs) - def rescan_hosts(self, hbas): for hba in hbas: self.echo_scsi_command("/sys/class/scsi_host/%s/scan" @@ -142,11 +137,6 @@ class LinuxFibreChannel(linuxscsi.LinuxSCSI): class LinuxFibreChannelS390X(LinuxFibreChannel): - def __init__(self, root_helper, execute=putils.execute, - *args, **kwargs): - super(LinuxFibreChannelS390X, self).__init__(root_helper, execute, - *args, **kwargs) - def get_fc_hbas_info(self): """Get Fibre Channel WWNs and device paths from the system, if any.""" diff --git a/os_brick/initiator/linuxscsi.py b/os_brick/initiator/linuxscsi.py index 7479a254f..b891b66d7 100644 --- a/os_brick/initiator/linuxscsi.py +++ b/os_brick/initiator/linuxscsi.py @@ -39,11 +39,6 @@ MULTIPATH_DEVICE_ACTIONS = ['unchanged:', 'reject:', 'reload:', class LinuxSCSI(executor.Executor): - def __init__(self, root_helper, execute=putils.execute, - *args, **kwargs): - super(LinuxSCSI, self).__init__(root_helper, execute, - *args, **kwargs) - def echo_scsi_command(self, path, content): """Used to echo strings to scsi subsystem.""" @@ -109,7 +104,7 @@ class LinuxSCSI(executor.Executor): def get_scsi_wwn(self, path): """Read the WWN from page 0x83 value for a SCSI device.""" - (out, _err) = self._execute('scsi_id', '--page', '0x83', + (out, _err) = self._execute('/lib/udev/scsi_id', '--page', '0x83', '--whitelisted', path, run_as_root=True, root_helper=self._root_helper) diff --git a/os_brick/local_dev/lvm.py b/os_brick/local_dev/lvm.py index fb55b4502..3b239d0cf 100644 --- a/os_brick/local_dev/lvm.py +++ b/os_brick/local_dev/lvm.py @@ -24,6 +24,7 @@ import re from os_brick import exception from os_brick import executor from os_brick.i18n import _LE, _LI +from os_brick.privileged import rootwrap as priv_rootwrap from os_brick import utils from oslo_concurrency import processutils as putils from oslo_log import log as logging @@ -41,7 +42,7 @@ class LVM(executor.Executor): def __init__(self, vg_name, root_helper, create_vg=False, physical_volumes=None, lvm_type='default', - executor=putils.execute, lvm_conf=None): + executor=None, lvm_conf=None): """Initialize the LVM object. @@ -197,9 +198,9 @@ class LVM(executor.Executor): """ cmd = LVM.LVM_CMD_PREFIX + ['vgs', '--version'] - (out, _err) = putils.execute(*cmd, - root_helper=root_helper, - run_as_root=True) + (out, _err) = priv_rootwrap.execute(*cmd, + root_helper=root_helper, + run_as_root=True) lines = out.split('\n') for line in lines: @@ -277,9 +278,9 @@ class LVM(executor.Executor): cmd.append(vg_name) try: - (out, _err) = putils.execute(*cmd, - root_helper=root_helper, - run_as_root=True) + (out, _err) = priv_rootwrap.execute(*cmd, + root_helper=root_helper, + run_as_root=True) except putils.ProcessExecutionError as err: with excutils.save_and_reraise_exception(reraise=True) as ctx: if "not found" in err.stderr or "Failed to find" in err.stderr: @@ -335,9 +336,9 @@ class LVM(executor.Executor): '-o', 'vg_name,name,size,free', '--separator', field_sep, '--nosuffix'] - (out, _err) = putils.execute(*cmd, - root_helper=root_helper, - run_as_root=True) + (out, _err) = priv_rootwrap.execute(*cmd, + root_helper=root_helper, + run_as_root=True) pvs = out.split() if vg_name is not None: @@ -379,9 +380,9 @@ class LVM(executor.Executor): if vg_name is not None: cmd.append(vg_name) - (out, _err) = putils.execute(*cmd, - root_helper=root_helper, - run_as_root=True) + (out, _err) = priv_rootwrap.execute(*cmd, + root_helper=root_helper, + run_as_root=True) vg_list = [] if out is not None: vgs = out.split() diff --git a/os_brick/privileged/__init__.py b/os_brick/privileged/__init__.py new file mode 100644 index 000000000..31efeb983 --- /dev/null +++ b/os_brick/privileged/__init__.py @@ -0,0 +1,23 @@ +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +from oslo_privsep import capabilities as c +from oslo_privsep import priv_context + +# It is expected that most (if not all) os-brick operations can be +# executed with these privileges. +default = priv_context.PrivContext( + __name__, + cfg_section='privsep_osbrick', + pypath=__name__ + '.default', + capabilities=[c.CAP_SYS_ADMIN], +) diff --git a/os_brick/privileged/rootwrap.py b/os_brick/privileged/rootwrap.py new file mode 100644 index 000000000..488eb7bb1 --- /dev/null +++ b/os_brick/privileged/rootwrap.py @@ -0,0 +1,82 @@ +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +"""Just in case it wasn't clear, this is a massive security back-door. + +`execute_root()` (or the same via `execute(run_as_root=True)`) allows +any command to be run as the privileged user (default "root"). This +is intended only as an expedient transition and should be removed +ASAP. + +This is not completely unreasonable because: + +1. We have no tool/workflow for merging changes to rootwrap filter + configs from os-brick into nova/cinder, which makes it difficult + to evolve these loosely coupled projects. + +2. Let's not pretend the earlier situation was any better. The + rootwrap filters config contained several entries like "allow cp as + root with any arguments", etc, and would have posed only a mild + inconvenience to an attacker. At least with privsep we can (in + principle) run the "root" commands as a non-root uid, with + restricted Linux capabilities. + +The plan is to switch os-brick to privsep using this module (removing +the urgency of (1)), then work on the larger refactor that addresses +(2) in followup changes. + +""" + +import six + +from oslo_concurrency import processutils as putils +from oslo_utils import strutils + +from os_brick import privileged + + +# Entrypoint used for rootwrap.py transition code. Don't use this for +# other purposes, since it will be removed when we think the +# transition is finished. +def execute(*cmd, **kwargs): + """NB: Raises processutils.ProcessExecutionError on failure.""" + run_as_root = kwargs.pop('run_as_root', False) + kwargs.pop('root_helper', None) + + try: + if run_as_root: + return execute_root(*cmd, **kwargs) + else: + return putils.execute(*cmd, **kwargs) + except OSError as e: + # Note: + # putils.execute('bogus', run_as_root=True) + # raises ProcessExecutionError(exit_code=1) (because there's a + # "sh -c bogus" involved in there somewhere, but: + # putils.execute('bogus', run_as_root=False) + # raises OSError(not found). + # + # Lots of code in os-brick catches only ProcessExecutionError + # and never encountered the latter when using rootwrap. + # Rather than fix all the callers, we just always raise + # ProcessExecutionError here :( + + sanitized_cmd = strutils.mask_password(' '.join(cmd)) + raise putils.ProcessExecutionError( + cmd=sanitized_cmd, description=six.text_type(e)) + + +# See comment on `execute` +@privileged.default.entrypoint +def execute_root(*cmd, **kwargs): + """NB: Raises processutils.ProcessExecutionError/OSError on failure.""" + return putils.execute(*cmd, shell=False, run_as_root=False, **kwargs) diff --git a/os_brick/remotefs/remotefs.py b/os_brick/remotefs/remotefs.py index 16b891beb..139362b7b 100644 --- a/os_brick/remotefs/remotefs.py +++ b/os_brick/remotefs/remotefs.py @@ -25,6 +25,7 @@ from oslo_log import log as logging import six from os_brick import exception +from os_brick.privileged import rootwrap as priv_rootwrap from os_brick.i18n import _, _LI LOG = logging.getLogger(__name__) @@ -33,7 +34,15 @@ LOG = logging.getLogger(__name__) class RemoteFsClient(object): def __init__(self, mount_type, root_helper, - execute=putils.execute, *args, **kwargs): + execute=None, *args, **kwargs): + # For backwards compatibility, `putils.execute` is interpreted + # as a sentinel to mean "I want the os-brick default" :-/ + # This can be burnt as soon as we update all the callsites (in + # nova+cinder) to the new default - and then we shall never + # speak of it again. + # TODO(gus): RemoteFsClient should probably inherit from Executor + if execute is None or execute == putils.execute: + execute = priv_rootwrap.execute mount_type_to_option_prefix = { 'nfs': 'nfs', diff --git a/os_brick/tests/initiator/test_connector.py b/os_brick/tests/initiator/test_connector.py index 9b49c606f..a7c92a6c9 100644 --- a/os_brick/tests/initiator/test_connector.py +++ b/os_brick/tests/initiator/test_connector.py @@ -36,6 +36,7 @@ from os_brick.initiator import linuxfc from os_brick.initiator import linuxrbd from os_brick.initiator import linuxscsi from os_brick.initiator import linuxsheepdog +from os_brick.privileged import rootwrap as priv_rootwrap from os_brick.remotefs import remotefs from os_brick.tests import base @@ -79,14 +80,14 @@ class ConnectorUtilsTestCase(base.TestCase): def test_brick_get_connector_properties(self): self._test_brick_get_connector_properties(False, False, False) - @mock.patch.object(putils, 'execute') + @mock.patch.object(priv_rootwrap, 'execute') def test_brick_get_connector_properties_multipath(self, mock_execute): self._test_brick_get_connector_properties(True, True, True) mock_execute.assert_called_once_with('multipathd', 'show', 'status', run_as_root=True, root_helper='sudo') - @mock.patch.object(putils, 'execute', + @mock.patch.object(priv_rootwrap, 'execute', side_effect=putils.ProcessExecutionError) def test_brick_get_connector_properties_fallback(self, mock_execute): self._test_brick_get_connector_properties(True, False, False) @@ -94,7 +95,7 @@ class ConnectorUtilsTestCase(base.TestCase): run_as_root=True, root_helper='sudo') - @mock.patch.object(putils, 'execute', + @mock.patch.object(priv_rootwrap, 'execute', side_effect=putils.ProcessExecutionError) def test_brick_get_connector_properties_raise(self, mock_execute): self.assertRaises(putils.ProcessExecutionError, @@ -419,7 +420,7 @@ class ISCSIConnectorTestCase(ConnectorTestCase): ('iscsiadm -m node -T %s -p %s --login' % (iqn, location)), ('iscsiadm -m node -T %s -p %s --op update' ' -n node.startup -v automatic' % (iqn, location)), - ('scsi_id --page 0x83 --whitelisted %s' % dev_str), + ('/lib/udev/scsi_id --page 0x83 --whitelisted %s' % dev_str), ('blockdev --flushbufs /dev/sdb'), ('tee -a /sys/block/sdb/device/delete'), ('iscsiadm -m node -T %s -p %s --op update' diff --git a/os_brick/tests/local_dev/test_brick_lvm.py b/os_brick/tests/local_dev/test_brick_lvm.py index 09002250c..2a462d889 100644 --- a/os_brick/tests/local_dev/test_brick_lvm.py +++ b/os_brick/tests/local_dev/test_brick_lvm.py @@ -17,6 +17,7 @@ from oslo_concurrency import processutils from os_brick import exception from os_brick.local_dev import lvm as brick +from os_brick.privileged import rootwrap as priv_rootwrap from os_brick.tests import base @@ -27,7 +28,7 @@ class BrickLvmTestCase(base.TestCase): self.volume_group_name = 'fake-vg' # Stub processutils.execute for static methods - self.mock_object(processutils, 'execute', + self.mock_object(priv_rootwrap, 'execute', self.fake_execute) self.vg = brick.LVM(self.volume_group_name, 'sudo', @@ -232,30 +233,30 @@ class BrickLvmTestCase(base.TestCase): # use the self._executor fake we pass in on init # so we need to stub processutils.execute appropriately - with mock.patch.object(processutils, 'execute', + with mock.patch.object(priv_rootwrap, 'execute', side_effect=self.fake_execute): self.assertTrue(self.vg.supports_thin_provisioning('sudo')) - with mock.patch.object(processutils, 'execute', + with mock.patch.object(priv_rootwrap, 'execute', side_effect=self.fake_pretend_lvm_version): self.assertTrue(self.vg.supports_thin_provisioning('sudo')) - with mock.patch.object(processutils, 'execute', + with mock.patch.object(priv_rootwrap, 'execute', side_effect=self.fake_old_lvm_version): self.assertFalse(self.vg.supports_thin_provisioning('sudo')) - with mock.patch.object(processutils, 'execute', + with mock.patch.object(priv_rootwrap, 'execute', side_effect=self.fake_customised_lvm_version): self.assertTrue(self.vg.supports_thin_provisioning('sudo')) def test_snapshot_lv_activate_support(self): self.vg._supports_snapshot_lv_activation = None - with mock.patch.object(processutils, 'execute', + with mock.patch.object(priv_rootwrap, 'execute', side_effect=self.fake_execute): self.assertTrue(self.vg.supports_snapshot_lv_activation) self.vg._supports_snapshot_lv_activation = None - with mock.patch.object(processutils, 'execute', + with mock.patch.object(priv_rootwrap, 'execute', side_effect=self.fake_old_lvm_version): self.assertFalse(self.vg.supports_snapshot_lv_activation) @@ -265,12 +266,12 @@ class BrickLvmTestCase(base.TestCase): """Tests if lvchange -K is available via a lvm2 version check.""" self.vg._supports_lvchange_ignoreskipactivation = None - with mock.patch.object(processutils, 'execute', + with mock.patch.object(priv_rootwrap, 'execute', side_effect=self.fake_pretend_lvm_version): self.assertTrue(self.vg.supports_lvchange_ignoreskipactivation) self.vg._supports_lvchange_ignoreskipactivation = None - with mock.patch.object(processutils, 'execute', + with mock.patch.object(priv_rootwrap, 'execute', side_effect=self.fake_old_lvm_version): self.assertFalse(self.vg.supports_lvchange_ignoreskipactivation) diff --git a/os_brick/tests/privileged/__init__.py b/os_brick/tests/privileged/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/os_brick/tests/privileged/test_rootwrap.py b/os_brick/tests/privileged/test_rootwrap.py new file mode 100644 index 000000000..a5997b500 --- /dev/null +++ b/os_brick/tests/privileged/test_rootwrap.py @@ -0,0 +1,59 @@ +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +import mock + +from oslo_concurrency import processutils as putils + +from os_brick import privileged +from os_brick.privileged import rootwrap as priv_rootwrap +from os_brick.tests import base + + +class PrivRootwrapTestCase(base.TestCase): + def setUp(self): + super(PrivRootwrapTestCase, self).setUp() + + # Bypass privsep and run these simple functions in-process + # (allows reading back the modified state of mocks) + privileged.default.set_client_mode(False) + self.addCleanup(privileged.default.set_client_mode, True) + + @mock.patch('os_brick.privileged.rootwrap.execute_root') + @mock.patch('oslo_concurrency.processutils.execute') + def test_execute(self, mock_putils_exec, mock_exec_root): + priv_rootwrap.execute('echo', 'foo', run_as_root=False) + self.assertFalse(mock_exec_root.called) + + priv_rootwrap.execute('echo', 'foo', run_as_root=True, + root_helper='baz', check_exit_code=0) + mock_exec_root.assert_called_once_with( + 'echo', 'foo', check_exit_code=0) + + @mock.patch('oslo_concurrency.processutils.execute') + def test_execute_root(self, mock_putils_exec): + priv_rootwrap.execute_root('echo', 'foo', check_exit_code=0) + mock_putils_exec.assert_called_once_with( + 'echo', 'foo', check_exit_code=0, shell=False, run_as_root=False) + + # Exact exception isn't particularly important, but these + # should be errors: + self.assertRaises(TypeError, + priv_rootwrap.execute_root, 'foo', shell=True) + self.assertRaises(TypeError, + priv_rootwrap.execute_root, 'foo', run_as_root=True) + + @mock.patch('oslo_concurrency.processutils.execute', + side_effect=OSError(42, 'mock error')) + def test_oserror_raise(self, mock_putils_exec): + self.assertRaises(putils.ProcessExecutionError, + priv_rootwrap.execute, 'foo') diff --git a/os_brick/tests/remotefs/test_remotefs.py b/os_brick/tests/remotefs/test_remotefs.py index c70a7c2ae..0049e821c 100644 --- a/os_brick/tests/remotefs/test_remotefs.py +++ b/os_brick/tests/remotefs/test_remotefs.py @@ -16,9 +16,8 @@ import mock import os import tempfile -from oslo_concurrency import processutils as putils - from os_brick import exception +from os_brick.privileged import rootwrap as priv_rootwrap from os_brick.remotefs import remotefs from os_brick.tests import base @@ -27,13 +26,12 @@ class RemoteFsClientTestCase(base.TestCase): def setUp(self): super(RemoteFsClientTestCase, self).setUp() - self.mock_execute = mock.patch.object(putils, 'execute').start() + self.mock_execute = mock.patch.object(priv_rootwrap, 'execute').start() @mock.patch.object(remotefs.RemoteFsClient, '_read_mounts', return_value=[]) def test_cifs(self, mock_read_mounts): client = remotefs.RemoteFsClient("cifs", root_helper='true', - execute=putils.execute, smbfs_mount_point_base='/mnt') share = '10.0.0.1:/qwe' mount_point = client.get_mount_point(share) @@ -48,7 +46,6 @@ class RemoteFsClientTestCase(base.TestCase): return_value=[]) def test_vzstorage_by_cluster_name(self, mock_read_mounts): client = remotefs.RemoteFsClient("vzstorage", root_helper='true', - execute=putils.execute, vzstorage_mount_point_base='/mnt') share = 'qwe' cluster_name = share @@ -64,7 +61,6 @@ class RemoteFsClientTestCase(base.TestCase): return_value=[]) def test_vzstorage_with_auth(self, mock_read_mounts): client = remotefs.RemoteFsClient("vzstorage", root_helper='true', - execute=putils.execute, vzstorage_mount_point_base='/mnt') cluster_name = 'qwe' password = '123456' @@ -84,7 +80,6 @@ class RemoteFsClientTestCase(base.TestCase): return_value=[]) def test_vzstorage_with_mds_list(self, mock_read_mounts): client = remotefs.RemoteFsClient("vzstorage", root_helper='true', - execute=putils.execute, vzstorage_mount_point_base='/mnt') cluster_name = 'qwe' mds_list = ['10.0.0.1', '10.0.0.2'] @@ -113,7 +108,6 @@ class RemoteFsClientTestCase(base.TestCase): return_value=[]) def test_vzstorage_invalid_share(self, mock_read_mounts): client = remotefs.RemoteFsClient("vzstorage", root_helper='true', - execute=putils.execute, vzstorage_mount_point_base='/mnt') self.assertRaises(exception.BrickException, client.mount, ':') @@ -121,7 +115,6 @@ class RemoteFsClientTestCase(base.TestCase): return_value=[]) def test_nfs(self, mock_read_mounts): client = remotefs.RemoteFsClient("nfs", root_helper='true', - execute=putils.execute, nfs_mount_point_base='/mnt') share = '10.0.0.1:/qwe' mount_point = client.get_mount_point(share) @@ -135,20 +128,19 @@ class RemoteFsClientTestCase(base.TestCase): def test_read_mounts(self): mounts = """device1 on mnt_point1 device2 on mnt_point2 type ext4 opts""" - with mock.patch.object(putils, 'execute', return_value=[mounts, '']): + with mock.patch.object(priv_rootwrap, 'execute', + return_value=[mounts, '']): client = remotefs.RemoteFsClient("cifs", root_helper='true', - execute=putils.execute, smbfs_mount_point_base='/mnt') ret = client._read_mounts() self.assertEqual(ret, {'mnt_point1': 'device1', 'mnt_point2': 'device2'}) - @mock.patch.object(putils, 'execute') + @mock.patch.object(priv_rootwrap, 'execute') @mock.patch.object(remotefs.RemoteFsClient, '_do_mount') def test_mount_already_mounted(self, mock_do_mount, mock_execute): share = "10.0.0.1:/share" client = remotefs.RemoteFsClient("cifs", root_helper='true', - execute=putils.execute, smbfs_mount_point_base='/mnt') mounts = {client.get_mount_point(share): 'some_dev'} with mock.patch.object(client, '_read_mounts', @@ -160,7 +152,7 @@ class RemoteFsClientTestCase(base.TestCase): def _test_no_mount_point(self, fs_type): self.assertRaises(exception.InvalidParameterValue, remotefs.RemoteFsClient, - fs_type, root_helper='true', execute=putils.execute) + fs_type, root_helper='true') def test_no_mount_point_nfs(self): self._test_no_mount_point('nfs') @@ -180,7 +172,7 @@ class RemoteFsClientTestCase(base.TestCase): def test_invalid_fs(self): self.assertRaises(exception.ProtocolNotSupported, remotefs.RemoteFsClient, - 'my_fs', root_helper='true', execute=putils.execute) + 'my_fs', root_helper='true') def test_init_sets_mount_base(self): client = remotefs.RemoteFsClient("cifs", root_helper='true', diff --git a/requirements.txt b/requirements.txt index 1bc4554ea..12eea9205 100644 --- a/requirements.txt +++ b/requirements.txt @@ -9,6 +9,7 @@ oslo.concurrency>=3.5.0 # Apache-2.0 oslo.log>=1.14.0 # Apache-2.0 oslo.serialization>=1.10.0 # Apache-2.0 oslo.i18n>=2.1.0 # Apache-2.0 +oslo.privsep>=1.5.0 # Apache-2.0 oslo.service>=1.0.0 # Apache-2.0 oslo.utils>=3.5.0 # Apache-2.0 requests!=2.9.0,>=2.8.1 # Apache-2.0