From c89c1f53db288143ef95362971d8db86b8f978ff Mon Sep 17 00:00:00 2001 From: Rodolfo Alonso Hernandez Date: Tue, 24 Nov 2020 16:16:02 +0000 Subject: [PATCH] Remove rootwrap execution (1) Replace rootwrap execution with privsep context execution. This series of patches will progressively replace any rootwrap call. This patch replaces some "IpNetnsCommand" command execution methods. Change-Id: Ic5fdf221a2a2cd0951539b0e040d2a941feee287 Story: #2007686 Task: #41558 --- neutron/agent/l3/dvr_fip_ns.py | 2 +- neutron/agent/l3/dvr_local_router.py | 3 +- neutron/agent/l3/namespaces.py | 8 +-- neutron/agent/linux/external_process.py | 8 ++- neutron/agent/linux/ip_lib.py | 26 ++++---- neutron/agent/linux/l3_tc_lib.py | 3 +- neutron/agent/linux/utils.py | 51 ++++++++++------ neutron/agent/windows/utils.py | 2 +- neutron/privileged/agent/linux/utils.py | 35 +++++++++++ neutron/tests/common/net_helpers.py | 3 +- .../functional/agent/linux/test_tc_lib.py | 6 +- neutron/tests/unit/agent/l3/test_agent.py | 12 ++-- .../tests/unit/agent/l3/test_dvr_fip_ns.py | 5 +- .../tests/unit/agent/l3/test_dvr_snat_ns.py | 3 +- .../openvswitch_firewall/test_iptables.py | 6 +- .../unit/agent/linux/test_external_process.py | 12 ++-- neutron/tests/unit/agent/linux/test_ip_lib.py | 39 +++++++----- .../tests/unit/agent/linux/test_keepalived.py | 3 + .../tests/unit/agent/linux/test_l3_tc_lib.py | 15 +++-- neutron/tests/unit/agent/linux/test_utils.py | 18 ++++-- .../linuxbridge/agent/test_arp_protect.py | 60 ++++++++++++------- ...ootwrap-with-privsep-5b85f1ba83df9554.yaml | 17 ++++++ 22 files changed, 237 insertions(+), 100 deletions(-) create mode 100644 releasenotes/notes/replace-rootwrap-with-privsep-5b85f1ba83df9554.yaml diff --git a/neutron/agent/l3/dvr_fip_ns.py b/neutron/agent/l3/dvr_fip_ns.py index f910cd4e530..6aabaa357f6 100644 --- a/neutron/agent/l3/dvr_fip_ns.py +++ b/neutron/agent/l3/dvr_fip_ns.py @@ -192,7 +192,7 @@ class FipNamespace(namespaces.Namespace): self.agent_gateway_port = ex_gw_port cmd = ['sysctl', '-w', 'net.ipv4.conf.%s.proxy_arp=1' % interface_name] - ip_wrapper.netns.execute(cmd, check_exit_code=False) + ip_wrapper.netns.execute(cmd, check_exit_code=False, privsep_exec=True) def create(self): LOG.debug("DVR: add fip namespace: %s", self.name) diff --git a/neutron/agent/l3/dvr_local_router.py b/neutron/agent/l3/dvr_local_router.py index 2666ac76e78..39eb5db4675 100644 --- a/neutron/agent/l3/dvr_local_router.py +++ b/neutron/agent/l3/dvr_local_router.py @@ -804,7 +804,8 @@ class DvrLocalRouter(dvr_router_base.DvrRouterBase): 'via', route['nexthop'], 'table', tbl_index] ip_wrapper = ip_lib.IPWrapper(namespace=fip_ns_name) if ip_wrapper.netns.exists(fip_ns_name): - ip_wrapper.netns.execute(cmd, check_exit_code=False) + ip_wrapper.netns.execute(cmd, check_exit_code=False, + privsep_exec=True) else: LOG.debug("The FIP namespace %(ns)s does not exist for " "router %(id)s", diff --git a/neutron/agent/l3/namespaces.py b/neutron/agent/l3/namespaces.py index d9916fde94a..d36fdd53cc6 100644 --- a/neutron/agent/l3/namespaces.py +++ b/neutron/agent/l3/namespaces.py @@ -94,18 +94,18 @@ class Namespace(object): # these sysctl values. ip_wrapper = self.ip_wrapper_root.ensure_namespace(self.name) cmd = ['sysctl', '-w', 'net.ipv4.ip_forward=1'] - ip_wrapper.netns.execute(cmd) + ip_wrapper.netns.execute(cmd, privsep_exec=True) # 1. Reply only if the target IP address is local address configured # on the incoming interface; and # 2. Always use the best local address cmd = ['sysctl', '-w', 'net.ipv4.conf.all.arp_ignore=1'] - ip_wrapper.netns.execute(cmd) + ip_wrapper.netns.execute(cmd, privsep_exec=True) cmd = ['sysctl', '-w', 'net.ipv4.conf.all.arp_announce=2'] - ip_wrapper.netns.execute(cmd) + ip_wrapper.netns.execute(cmd, privsep_exec=True) if self.use_ipv6: cmd = ['sysctl', '-w', 'net.ipv6.conf.all.forwarding=%d' % int(ipv6_forwarding)] - ip_wrapper.netns.execute(cmd) + ip_wrapper.netns.execute(cmd, privsep_exec=True) def delete(self): try: diff --git a/neutron/agent/linux/external_process.py b/neutron/agent/linux/external_process.py index e45a862a224..20bb089b1d2 100644 --- a/neutron/agent/linux/external_process.py +++ b/neutron/agent/linux/external_process.py @@ -98,7 +98,7 @@ class ProcessManager(MonitoredProcess): else: self.disable('HUP') - def disable(self, sig='9', get_stop_command=None): + def disable(self, sig='9', get_stop_command=None, privsep_exec=False): pid = self.pid if self.active: @@ -106,10 +106,12 @@ class ProcessManager(MonitoredProcess): cmd = get_stop_command(self.get_pid_file_name()) ip_wrapper = ip_lib.IPWrapper(namespace=self.namespace) ip_wrapper.netns.execute(cmd, addl_env=self.cmd_addl_env, - run_as_root=self.run_as_root) + run_as_root=self.run_as_root, + privsep_exec=privsep_exec) else: cmd = self.get_kill_cmd(sig, pid) - utils.execute(cmd, run_as_root=self.run_as_root) + utils.execute(cmd, run_as_root=self.run_as_root, + privsep_exec=privsep_exec) # In the case of shutting down, remove the pid file if sig == '9': utils.delete_if_exists(self.get_pid_file_name(), diff --git a/neutron/agent/linux/ip_lib.py b/neutron/agent/linux/ip_lib.py index 8bb67444550..28c191fc8f7 100644 --- a/neutron/agent/linux/ip_lib.py +++ b/neutron/agent/linux/ip_lib.py @@ -354,7 +354,7 @@ class IPDevice(SubProcessBase): try: ip_wrapper.netns.execute(["conntrack", "-D", "-d", ip_str], check_exit_code=True, - extra_ok_codes=[1]) + extra_ok_codes=[1], privsep_exec=True) except RuntimeError: LOG.exception("Failed deleting ingress connection state of" @@ -364,7 +364,7 @@ class IPDevice(SubProcessBase): try: ip_wrapper.netns.execute(["conntrack", "-D", "-q", ip_str], check_exit_code=True, - extra_ok_codes=[1]) + extra_ok_codes=[1], privsep_exec=True) except RuntimeError: LOG.exception("Failed deleting egress connection state of" " floatingip %s", ip_str) @@ -376,7 +376,7 @@ class IPDevice(SubProcessBase): '--dport', dport] try: ip_wrapper.netns.execute(cmd, check_exit_code=True, - extra_ok_codes=[1]) + extra_ok_codes=[1], privsep_exec=True) except RuntimeError: LOG.exception("Failed deleting ingress connection state of " @@ -699,7 +699,8 @@ class IpNetnsCommand(IpCommandBase): create_network_namespace(name) wrapper = IPWrapper(namespace=name) wrapper.netns.execute(['sysctl', '-w', - 'net.ipv4.conf.all.promote_secondaries=1']) + 'net.ipv4.conf.all.promote_secondaries=1'], + privsep_exec=True) return wrapper def delete(self, name): @@ -707,7 +708,7 @@ class IpNetnsCommand(IpCommandBase): def execute(self, cmds, addl_env=None, check_exit_code=True, log_fail_as_error=True, extra_ok_codes=None, - run_as_root=False): + run_as_root=False, privsep_exec=False): ns_params = [] if self._parent.namespace: run_as_root = True @@ -721,7 +722,8 @@ class IpNetnsCommand(IpCommandBase): return utils.execute(cmd, check_exit_code=check_exit_code, extra_ok_codes=extra_ok_codes, log_fail_as_error=log_fail_as_error, - run_as_root=run_as_root) + run_as_root=run_as_root, + privsep_exec=privsep_exec) def exists(self, name): return network_namespace_exists(name) @@ -1014,7 +1016,8 @@ def _arping(ns_name, iface_name, address, count, log_exception): '-w', 2, address] try: ip_wrapper.netns.execute(arping_cmd, - extra_ok_codes=extra_ok_codes) + extra_ok_codes=extra_ok_codes, + privsep_exec=True) except Exception as exc: # Since this is spawned in a thread and executed 2 seconds # apart, something may have been deleted while we were @@ -1101,7 +1104,8 @@ def sysctl(cmd, namespace=None, log_fail_as_error=True): ip_wrapper = IPWrapper(namespace=namespace) try: ip_wrapper.netns.execute(cmd, run_as_root=True, - log_fail_as_error=log_fail_as_error) + log_fail_as_error=log_fail_as_error, + privsep_exec=True) except RuntimeError as rte: LOG.warning( "Setting %(cmd)s in namespace %(ns)s failed: %(err)s.", @@ -1127,7 +1131,8 @@ def get_ip_nonlocal_bind(namespace=None): """Get kernel option value of ip_nonlocal_bind in given namespace.""" cmd = ['sysctl', '-bn', IP_NONLOCAL_BIND] ip_wrapper = IPWrapper(namespace) - return int(ip_wrapper.netns.execute(cmd, run_as_root=True)) + return int(ip_wrapper.netns.execute(cmd, run_as_root=True, + privsep_exec=True)) def set_ip_nonlocal_bind(value, namespace=None, log_fail_as_error=True): @@ -1162,7 +1167,8 @@ def get_ipv6_forwarding(device, namespace=None): """Get kernel value of IPv6 forwarding for device in given namespace.""" cmd = ['sysctl', '-b', "net.ipv6.conf.%s.forwarding" % device] ip_wrapper = IPWrapper(namespace) - return int(ip_wrapper.netns.execute(cmd, run_as_root=True)) + return int(ip_wrapper.netns.execute(cmd, run_as_root=True, + privsep_exec=True)) def _parse_ip_rule(rule, ip_version): diff --git a/neutron/agent/linux/l3_tc_lib.py b/neutron/agent/linux/l3_tc_lib.py index b0b73728841..f8fd7177e17 100644 --- a/neutron/agent/linux/l3_tc_lib.py +++ b/neutron/agent/linux/l3_tc_lib.py @@ -33,7 +33,8 @@ class FloatingIPTcCommandBase(ip_lib.IPDevice): def _execute_tc_cmd(self, cmd, **kwargs): cmd = ['tc'] + cmd ip_wrapper = ip_lib.IPWrapper(self.namespace) - return ip_wrapper.netns.execute(cmd, run_as_root=True, **kwargs) + return ip_wrapper.netns.execute(cmd, run_as_root=True, + privsep_exec=True, **kwargs) def _get_qdisc_id_for_filter(self, direction): qdiscs = tc_lib.list_tc_qdiscs(self.name, namespace=self.namespace) diff --git a/neutron/agent/linux/utils.py b/neutron/agent/linux/utils.py index e29c770d61d..bf2a05bc2bd 100644 --- a/neutron/agent/linux/utils.py +++ b/neutron/agent/linux/utils.py @@ -35,7 +35,6 @@ from oslo_utils import excutils from oslo_utils import fileutils import psutil -from neutron.common import utils from neutron.conf.agent import common as config from neutron.privileged.agent.linux import utils as priv_utils from neutron import wsgi @@ -79,16 +78,26 @@ def create_process(cmd, run_as_root=False, addl_env=None): """ cmd = list(map(str, addl_env_args(addl_env) + cmd)) if run_as_root: + # NOTE(ralonsoh): to be removed once the migration to privsep + # execution is done. cmd = shlex.split(config.get_root_helper(cfg.CONF)) + cmd LOG.debug("Running command: %s", cmd) - obj = utils.subprocess_popen(cmd, shell=False, - stdin=subprocess.PIPE, - stdout=subprocess.PIPE, - stderr=subprocess.PIPE) + obj = subprocess.Popen(cmd, shell=False, stdin=subprocess.PIPE, + stdout=subprocess.PIPE, stderr=subprocess.PIPE) return obj, cmd +def _execute_process(cmd, _process_input, addl_env, run_as_root): + obj, cmd = create_process(cmd, run_as_root=run_as_root, addl_env=addl_env) + _stdout, _stderr = obj.communicate(_process_input) + returncode = obj.returncode + obj.stdin.close() + _stdout = helpers.safe_decode_utf8(_stdout) + _stderr = helpers.safe_decode_utf8(_stderr) + return _stdout, _stderr, returncode + + def execute_rootwrap_daemon(cmd, process_input, addl_env): cmd = list(map(str, addl_env_args(addl_env) + cmd)) # NOTE(twilson) oslo_rootwrap.daemon will raise on filter match @@ -99,31 +108,34 @@ def execute_rootwrap_daemon(cmd, process_input, addl_env): LOG.debug("Running command (rootwrap daemon): %s", cmd) client = RootwrapDaemonHelper.get_client() try: - return client.execute(cmd, process_input) + returncode, _stdout, _stderr = client.execute(cmd, process_input) except Exception: with excutils.save_and_reraise_exception(): LOG.error("Rootwrap error running command: %s", cmd) + _stdout = helpers.safe_decode_utf8(_stdout) + _stderr = helpers.safe_decode_utf8(_stderr) + return _stdout, _stderr, returncode + def execute(cmd, process_input=None, addl_env=None, check_exit_code=True, return_stderr=False, log_fail_as_error=True, - extra_ok_codes=None, run_as_root=False): + extra_ok_codes=None, run_as_root=False, privsep_exec=False): try: if process_input is not None: _process_input = encodeutils.to_utf8(process_input) else: _process_input = None - if run_as_root and cfg.CONF.AGENT.root_helper_daemon: - returncode, _stdout, _stderr = ( - execute_rootwrap_daemon(cmd, process_input, addl_env)) + + if run_as_root and privsep_exec: + _stdout, _stderr, returncode = priv_utils.execute_process( + cmd, _process_input, addl_env) + elif run_as_root and cfg.CONF.AGENT.root_helper_daemon: + _stdout, _stderr, returncode = execute_rootwrap_daemon( + cmd, process_input, addl_env) else: - obj, cmd = create_process(cmd, run_as_root=run_as_root, - addl_env=addl_env) - _stdout, _stderr = obj.communicate(_process_input) - returncode = obj.returncode - obj.stdin.close() - _stdout = helpers.safe_decode_utf8(_stdout) - _stderr = helpers.safe_decode_utf8(_stderr) + _stdout, _stderr, returncode = _execute_process( + cmd, _process_input, addl_env, run_as_root) extra_ok_codes = extra_ok_codes or [] if returncode and returncode not in extra_ok_codes: @@ -202,10 +214,11 @@ def find_fork_top_parent(pid): return pid -def kill_process(pid, signal, run_as_root=False): +def kill_process(pid, signal, run_as_root=False, privsep_exec=False): """Kill the process with the given pid using the given signal.""" try: - execute(['kill', '-%d' % signal, pid], run_as_root=run_as_root) + execute(['kill', '-%d' % signal, pid], run_as_root=run_as_root, + privsep_exec=privsep_exec) except exceptions.ProcessExecutionError: if process_is_running(pid): raise diff --git a/neutron/agent/windows/utils.py b/neutron/agent/windows/utils.py index adf76b2877b..e2d5aa1cd7a 100644 --- a/neutron/agent/windows/utils.py +++ b/neutron/agent/windows/utils.py @@ -100,7 +100,7 @@ def kill_process(pid, signal, run_as_root=False): def execute(cmd, process_input=None, addl_env=None, check_exit_code=True, return_stderr=False, log_fail_as_error=True, - extra_ok_codes=None, run_as_root=False): + extra_ok_codes=None, run_as_root=False, privsep_exec=False): if process_input is not None: _process_input = encodeutils.to_utf8(process_input) diff --git a/neutron/privileged/agent/linux/utils.py b/neutron/privileged/agent/linux/utils.py index edceb643d61..a87edf907bf 100644 --- a/neutron/privileged/agent/linux/utils.py +++ b/neutron/privileged/agent/linux/utils.py @@ -15,6 +15,8 @@ import os import re +from eventlet.green import subprocess +from neutron_lib.utils import helpers from oslo_concurrency import processutils from oslo_utils import fileutils @@ -47,3 +49,36 @@ def _find_listen_pids_namespace(namespace): @privileged.default.entrypoint def delete_if_exists(path, remove=os.unlink): fileutils.delete_if_exists(path, remove=remove) + + +@privileged.default.entrypoint +def execute_process(cmd, _process_input, addl_env): + obj, cmd = _create_process(cmd, addl_env=addl_env) + _stdout, _stderr = obj.communicate(_process_input) + returncode = obj.returncode + obj.stdin.close() + _stdout = helpers.safe_decode_utf8(_stdout) + _stderr = helpers.safe_decode_utf8(_stderr) + return _stdout, _stderr, returncode + + +def _addl_env_args(addl_env): + """Build arguments for adding additional environment vars with env""" + + # NOTE (twilson) If using rootwrap, an EnvFilter should be set up for the + # command instead of a CommandFilter. + if addl_env is None: + return [] + return ['env'] + ['%s=%s' % pair for pair in addl_env.items()] + + +def _create_process(cmd, addl_env=None): + """Create a process object for the given command. + + The return value will be a tuple of the process object and the + list of command arguments used to create it. + """ + cmd = list(map(str, _addl_env_args(addl_env) + list(cmd))) + obj = subprocess.Popen(cmd, shell=False, stdin=subprocess.PIPE, + stdout=subprocess.PIPE, stderr=subprocess.PIPE) + return obj, cmd diff --git a/neutron/tests/common/net_helpers.py b/neutron/tests/common/net_helpers.py index 45a43fe50b2..4aca5ad4efb 100644 --- a/neutron/tests/common/net_helpers.py +++ b/neutron/tests/common/net_helpers.py @@ -620,7 +620,8 @@ class NamespaceFixture(fixtures.Fixture): if self.ip_wrapper.netns.exists(self.name): for pid in ip_lib.list_namespace_pids(self.name): utils.kill_process(pid, signal.SIGKILL, - run_as_root=True) + run_as_root=True, + privsep_exec=True) self.ip_wrapper.netns.delete(self.name) except helpers.TestTimerTimeout: LOG.warning('Namespace %s was not deleted due to a timeout.', diff --git a/neutron/tests/functional/agent/linux/test_tc_lib.py b/neutron/tests/functional/agent/linux/test_tc_lib.py index 3a48a858248..b4ce33c1471 100644 --- a/neutron/tests/functional/agent/linux/test_tc_lib.py +++ b/neutron/tests/functional/agent/linux/test_tc_lib.py @@ -134,8 +134,8 @@ class TcPolicyClassTestCase(functional_base.BaseSudoTestCase): bytes = tc_classes[0]['stats']['bytes'] packets = tc_classes[0]['stats']['packets'] - net_helpers.assert_ping(self.ns[1], netaddr.IPNetwork(self.ip[0]).ip, - count=1) + net_helpers.assert_ping( + self.ns[1], str(netaddr.IPNetwork(self.ip[0]).ip), count=1) tc_classes = tc_lib.list_tc_policy_class(self.device[0], namespace=self.ns[0]) self.assertGreater(tc_classes[0]['stats']['bytes'], bytes) @@ -268,7 +268,7 @@ class TcFiltersTestCase(functional_base.BaseSudoTestCase): 'device': self.device[0]}) net_helpers.assert_ping( - self.ns[1], netaddr.IPNetwork(self.ip_vxlan[0]).ip, count=1) + self.ns[1], str(netaddr.IPNetwork(self.ip_vxlan[0]).ip), count=1) tc_classes = tc_lib.list_tc_policy_class(self.device[0], namespace=self.ns[0]) for tc_class in tc_classes: diff --git a/neutron/tests/unit/agent/l3/test_agent.py b/neutron/tests/unit/agent/l3/test_agent.py index 9448a73b047..6b011a62f64 100644 --- a/neutron/tests/unit/agent/l3/test_agent.py +++ b/neutron/tests/unit/agent/l3/test_agent.py @@ -2595,13 +2595,17 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework): 'qrouter-bar', self.conf, agent.driver, agent.use_ipv6) ns.create() - calls = [mock.call(['sysctl', '-w', 'net.ipv4.ip_forward=1']), - mock.call(['sysctl', '-w', 'net.ipv4.conf.all.arp_ignore=1']), + calls = [mock.call(['sysctl', '-w', 'net.ipv4.ip_forward=1'], + privsep_exec=True), + mock.call(['sysctl', '-w', 'net.ipv4.conf.all.arp_ignore=1'], + privsep_exec=True), mock.call( - ['sysctl', '-w', 'net.ipv4.conf.all.arp_announce=2'])] + ['sysctl', '-w', 'net.ipv4.conf.all.arp_announce=2'], + privsep_exec=True)] if agent.use_ipv6: calls.append(mock.call( - ['sysctl', '-w', 'net.ipv6.conf.all.forwarding=1'])) + ['sysctl', '-w', 'net.ipv6.conf.all.forwarding=1'], + privsep_exec=True)) self.mock_ip.netns.execute.assert_has_calls(calls) diff --git a/neutron/tests/unit/agent/l3/test_dvr_fip_ns.py b/neutron/tests/unit/agent/l3/test_dvr_fip_ns.py index ea8ec763918..8ef3a96bba8 100644 --- a/neutron/tests/unit/agent/l3/test_dvr_fip_ns.py +++ b/neutron/tests/unit/agent/l3/test_dvr_fip_ns.py @@ -217,13 +217,14 @@ class TestDvrFipNs(base.BaseTestCase): bind_cmd = ['sysctl', '-w', 'net.ipv4.ip_nonlocal_bind=1'] expected = [mock.call(netns_cmd + bind_cmd, check_exit_code=True, extra_ok_codes=None, log_fail_as_error=False, - run_as_root=True)] + run_as_root=True, privsep_exec=True)] if old_kernel: expected.append(mock.call(bind_cmd, check_exit_code=True, extra_ok_codes=None, log_fail_as_error=True, - run_as_root=True)) + run_as_root=True, + privsep_exec=True)) execute.assert_has_calls(expected) diff --git a/neutron/tests/unit/agent/l3/test_dvr_snat_ns.py b/neutron/tests/unit/agent/l3/test_dvr_snat_ns.py index 0b967ac637f..9c156b997ac 100644 --- a/neutron/tests/unit/agent/l3/test_dvr_snat_ns.py +++ b/neutron/tests/unit/agent/l3/test_dvr_snat_ns.py @@ -50,7 +50,8 @@ class TestDvrSnatNs(base.BaseTestCase): loose_cmd = ['sysctl', '-w', 'net.netfilter.nf_conntrack_tcp_loose=0'] expected = [mock.call(netns_cmd + loose_cmd, check_exit_code=True, extra_ok_codes=None, - log_fail_as_error=True, run_as_root=True)] + log_fail_as_error=True, run_as_root=True, + privsep_exec=True)] create.assert_called_once_with(self.snat_ns.name) execute.assert_has_calls(expected) diff --git a/neutron/tests/unit/agent/linux/openvswitch_firewall/test_iptables.py b/neutron/tests/unit/agent/linux/openvswitch_firewall/test_iptables.py index a82f3c74041..eab88d4844d 100644 --- a/neutron/tests/unit/agent/linux/openvswitch_firewall/test_iptables.py +++ b/neutron/tests/unit/agent/linux/openvswitch_firewall/test_iptables.py @@ -110,8 +110,10 @@ class TestHybridIptablesHelper(base.BaseTestCase): def test_overloaded_remove_conntrack(self): with mock.patch.object(iptables_firewall.IptablesFirewallDriver, '_remove_conntrack_entries_from_port_deleted') as rcefpd, \ - mock.patch("neutron.agent.linux.ip_conntrack.IpConntrackManager" - "._populate_initial_zone_map"): + mock.patch("neutron.agent.linux.ip_conntrack." + "IpConntrackManager._populate_initial_zone_map"), \ + mock.patch.object(iptables_firewall.IptablesFirewallDriver, + '_check_netfilter_for_bridges'): firewall = iptables.get_iptables_driver_instance() firewall._remove_conntrack_entries_from_port_deleted(None) rcefpd.assert_not_called() diff --git a/neutron/tests/unit/agent/linux/test_external_process.py b/neutron/tests/unit/agent/linux/test_external_process.py index 5b7d54673a4..3f52eb8d52d 100644 --- a/neutron/tests/unit/agent/linux/test_external_process.py +++ b/neutron/tests/unit/agent/linux/test_external_process.py @@ -138,7 +138,8 @@ class TestProcessManager(base.BaseTestCase): check_exit_code=True, extra_ok_codes=None, run_as_root=False, - log_fail_as_error=True) + log_fail_as_error=True, + privsep_exec=False) def test_enable_with_namespace(self): callback = mock.Mock() @@ -228,7 +229,8 @@ class TestProcessManager(base.BaseTestCase): manager.disable() utils.assert_has_calls([ mock.call.execute(['kill', '-9', 4], - run_as_root=False)]) + run_as_root=False, + privsep_exec=False)]) def test_disable_namespace(self): with mock.patch.object(ep.ProcessManager, 'pid') as pid: @@ -242,7 +244,8 @@ class TestProcessManager(base.BaseTestCase): manager.disable() utils.assert_has_calls([ mock.call.execute(['kill', '-9', 4], - run_as_root=True)]) + run_as_root=True, + privsep_exec=False)]) def test_disable_not_active(self): with mock.patch.object(ep.ProcessManager, 'pid') as pid: @@ -284,7 +287,8 @@ class TestProcessManager(base.BaseTestCase): return_value=kill_script_exists): manager.disable() utils.execute.assert_called_with( - expected_cmd, run_as_root=bool(namespace)) + expected_cmd, run_as_root=bool(namespace), + privsep_exec=False) def test_disable_custom_kill_script_no_namespace(self): self._test_disable_custom_kill_script( diff --git a/neutron/tests/unit/agent/linux/test_ip_lib.py b/neutron/tests/unit/agent/linux/test_ip_lib.py index 62798315cd3..8f130fff000 100644 --- a/neutron/tests/unit/agent/linux/test_ip_lib.py +++ b/neutron/tests/unit/agent/linux/test_ip_lib.py @@ -858,7 +858,7 @@ class TestIpNetnsCommand(TestIPCmdBase): ['ip', 'netns', 'exec', 'ns', 'sysctl', '-w', 'net.ipv4.conf.all.promote_secondaries=1'], run_as_root=True, check_exit_code=True, extra_ok_codes=None, - log_fail_as_error=True) + log_fail_as_error=True, privsep_exec=True) @mock.patch.object(priv_lib, 'remove_netns') def test_delete_namespace(self, remove): @@ -899,7 +899,8 @@ class TestIpNetnsCommand(TestIPCmdBase): run_as_root=True, check_exit_code=True, extra_ok_codes=None, - log_fail_as_error=True) + log_fail_as_error=True, + privsep_exec=False) def test_execute_env_var_prepend(self): self.parent.namespace = 'ns' @@ -911,7 +912,7 @@ class TestIpNetnsCommand(TestIPCmdBase): ['%s=%s' % (k, v) for k, v in env.items()] + ['ip', 'link', 'list'], run_as_root=True, check_exit_code=True, extra_ok_codes=None, - log_fail_as_error=True) + log_fail_as_error=True, privsep_exec=False) def test_execute_nosudo_with_no_namespace(self): with mock.patch('neutron.agent.common.utils.execute') as execute: @@ -921,7 +922,8 @@ class TestIpNetnsCommand(TestIPCmdBase): check_exit_code=True, extra_ok_codes=None, run_as_root=False, - log_fail_as_error=True) + log_fail_as_error=True, + privsep_exec=False) class TestDeviceExists(base.BaseTestCase): @@ -1378,12 +1380,18 @@ class TestArpPing(TestIPCmdBase): self.assertTrue(spawn_n.called) mIPWrapper.assert_has_calls([ mock.call(namespace=mock.sentinel.ns_name), - mock.call().netns.execute(mock.ANY, extra_ok_codes=[1]), - mock.call().netns.execute(mock.ANY, extra_ok_codes=[1]), - mock.call().netns.execute(mock.ANY, extra_ok_codes=[1, 2]), - mock.call().netns.execute(mock.ANY, extra_ok_codes=[1, 2]), - mock.call().netns.execute(mock.ANY, extra_ok_codes=[1, 2]), - mock.call().netns.execute(mock.ANY, extra_ok_codes=[1, 2])]) + mock.call().netns.execute(mock.ANY, extra_ok_codes=[1], + privsep_exec=True), + mock.call().netns.execute(mock.ANY, extra_ok_codes=[1], + privsep_exec=True), + mock.call().netns.execute(mock.ANY, extra_ok_codes=[1, 2], + privsep_exec=True), + mock.call().netns.execute(mock.ANY, extra_ok_codes=[1, 2], + privsep_exec=True), + mock.call().netns.execute(mock.ANY, extra_ok_codes=[1, 2], + privsep_exec=True), + mock.call().netns.execute(mock.ANY, extra_ok_codes=[1, 2], + privsep_exec=True)]) ip_wrapper = mIPWrapper(namespace=mock.sentinel.ns_name) @@ -1395,7 +1403,8 @@ class TestArpPing(TestIPCmdBase): '-w', mock.ANY, address] ip_wrapper.netns.execute.assert_any_call(arping_cmd, - extra_ok_codes=mock.ANY) + extra_ok_codes=mock.ANY, + privsep_exec=True) @mock.patch.object(ip_lib, 'IPWrapper') @mock.patch('eventlet.spawn_n') @@ -1415,7 +1424,8 @@ class TestArpPing(TestIPCmdBase): # should return early with a single call when ENODEV mIPWrapper.assert_has_calls([ mock.call(namespace=mock.sentinel.ns_name), - mock.call().netns.execute(mock.ANY, extra_ok_codes=[1]) + mock.call().netns.execute(mock.ANY, extra_ok_codes=[1], + privsep_exec=True) ] * 1) @mock.patch('eventlet.spawn_n') @@ -1458,7 +1468,7 @@ class TestSysctl(base.BaseTestCase): dev.disable_ipv6() self.execute.assert_called_once_with( ['sysctl', '-w', 'net.ipv6.conf.tap0.disable_ipv6=1'], - log_fail_as_error=True, run_as_root=True) + log_fail_as_error=True, run_as_root=True, privsep_exec=True) def test_disable_ipv6_when_ipv6_globally_disabled(self): dev = ip_lib.IPDevice('tap0', 'ns1') @@ -1483,7 +1493,8 @@ class TestConntrack(base.BaseTestCase): '--dport', dport] device.delete_socket_conntrack_state(ip_str, dport, protocol) self.execute.assert_called_once_with(expect_cmd, check_exit_code=True, - extra_ok_codes=[1]) + extra_ok_codes=[1], + privsep_exec=True) class ParseIpRuleTestCase(base.BaseTestCase): diff --git a/neutron/tests/unit/agent/linux/test_keepalived.py b/neutron/tests/unit/agent/linux/test_keepalived.py index cee45180e6a..28474f9dc89 100644 --- a/neutron/tests/unit/agent/linux/test_keepalived.py +++ b/neutron/tests/unit/agent/linux/test_keepalived.py @@ -47,6 +47,8 @@ class KeepalivedBaseTestCase(base.BaseTestCase): def setUp(self): super(KeepalivedBaseTestCase, self).setUp() l3_config.register_l3_agent_config_opts(l3_config.OPTS, cfg.CONF) + self._mock_no_track_supported = mock.patch.object( + keepalived, '_is_keepalived_use_no_track_supported') class KeepalivedGetFreeRangeTestCase(KeepalivedBaseTestCase): @@ -193,6 +195,7 @@ class KeepalivedConfTestCase(KeepalivedBaseTestCase, self.assertEqual(self.expected, config.get_config_str()) def test_config_generation_no_track_not_supported(self): + self._mock_no_track_supported.start().return_value = False config = self._get_config() with mock.patch.object( keepalived, '_is_keepalived_use_no_track_supported', diff --git a/neutron/tests/unit/agent/linux/test_l3_tc_lib.py b/neutron/tests/unit/agent/linux/test_l3_tc_lib.py index fecbe7bd190..34c6e1ad41d 100644 --- a/neutron/tests/unit/agent/linux/test_l3_tc_lib.py +++ b/neutron/tests/unit/agent/linux/test_l3_tc_lib.py @@ -171,7 +171,8 @@ class TestFloatingIPTcCommandBase(base.BaseTestCase): run_as_root=True, check_exit_code=True, log_fail_as_error=True, - extra_ok_codes=None + extra_ok_codes=None, + privsep_exec=True ) def _test__get_filterid_for_ip(self, filters): @@ -236,7 +237,8 @@ class TestFloatingIPTcCommandBase(base.BaseTestCase): run_as_root=True, check_exit_code=True, log_fail_as_error=True, - extra_ok_codes=None + extra_ok_codes=None, + privsep_exec=True ) def _test__get_qdisc_filters(self, filters): @@ -277,7 +279,8 @@ class TestFloatingIPTcCommandBase(base.BaseTestCase): run_as_root=True, check_exit_code=True, log_fail_as_error=True, - extra_ok_codes=None + extra_ok_codes=None, + privsep_exec=True ) def test__get_or_create_qdisc(self): @@ -382,7 +385,8 @@ class TestFloatingIPTcCommand(base.BaseTestCase): run_as_root=True, check_exit_code=True, log_fail_as_error=True, - extra_ok_codes=None + extra_ok_codes=None, + privsep_exec=True ) def test_set_ip_rate_limit_no_qdisc_without_chain(self): @@ -409,7 +413,8 @@ class TestFloatingIPTcCommand(base.BaseTestCase): run_as_root=True, check_exit_code=True, log_fail_as_error=True, - extra_ok_codes=None + extra_ok_codes=None, + privsep_exec=True ) def test_get_filter_id_for_ip(self): diff --git a/neutron/tests/unit/agent/linux/test_utils.py b/neutron/tests/unit/agent/linux/test_utils.py index 68938726f71..4dab58292f7 100644 --- a/neutron/tests/unit/agent/linux/test_utils.py +++ b/neutron/tests/unit/agent/linux/test_utils.py @@ -25,6 +25,7 @@ from oslo_config import cfg import oslo_i18n from neutron.agent.linux import utils +from neutron.privileged.agent.linux import utils as priv_utils from neutron.tests import base @@ -46,7 +47,15 @@ class AgentUtilsExecuteTest(base.BaseTestCase): result = utils.execute(["ls", self.test_file]) self.assertEqual(result, expected) - def test_with_helper(self): + def test_with_root_privileges_privsep(self): + with mock.patch.object(priv_utils, 'execute_process') as \ + mock_execute_process: + mock_execute_process.return_value = ['', '', 0] + utils.execute(['ls', self.test_file], run_as_root=True, + privsep_exec=True) + mock_execute_process.assert_called_once() + + def test_with_root_privileges_rootwrap(self): expected = "ls %s\n" % self.test_file self.mock_popen.return_value = [expected, ""] self.config(group='AGENT', root_helper='echo') @@ -54,7 +63,7 @@ class AgentUtilsExecuteTest(base.BaseTestCase): self.assertEqual(result, expected) @mock.patch.object(utils.RootwrapDaemonHelper, 'get_client') - def test_with_helper_exception(self, get_client): + def test_with_root_privileges_rootwrap_exception(self, get_client): client_inst = mock.Mock() client_inst.execute.side_effect = RuntimeError get_client.return_value = client_inst @@ -229,10 +238,11 @@ class TestKillProcess(base.BaseTestCase): side_effect=exc) as mock_execute: with mock.patch.object(utils, 'process_is_running', return_value=not pid_killed): - utils.kill_process(pid, kill_signal, run_as_root=True) + utils.kill_process(pid, kill_signal, run_as_root=True, + privsep_exec=False) mock_execute.assert_called_with(['kill', '-%d' % kill_signal, pid], - run_as_root=True) + run_as_root=True, privsep_exec=False) def test_kill_process_returns_none_for_valid_pid(self): self._test_kill_process('1') diff --git a/neutron/tests/unit/plugins/ml2/drivers/linuxbridge/agent/test_arp_protect.py b/neutron/tests/unit/plugins/ml2/drivers/linuxbridge/agent/test_arp_protect.py index a1679d1bb57..41d2ac703c3 100644 --- a/neutron/tests/unit/plugins/ml2/drivers/linuxbridge/agent/test_arp_protect.py +++ b/neutron/tests/unit/plugins/ml2/drivers/linuxbridge/agent/test_arp_protect.py @@ -66,34 +66,40 @@ class TestLinuxBridgeARPSpoofing(base.BaseTestCase): expected = [ mock.call(['ebtables', '-t', 'nat', '--concurrent', '-L'], check_exit_code=True, extra_ok_codes=None, - log_fail_as_error=True, run_as_root=True), + log_fail_as_error=True, run_as_root=True, + privsep_exec=False), mock.ANY, mock.ANY, mock.call(['ebtables', '-t', 'nat', '--concurrent', '-N', 'neutronMAC-%s' % vif, '-P', 'DROP'], check_exit_code=True, extra_ok_codes=None, - log_fail_as_error=True, run_as_root=True), + log_fail_as_error=True, run_as_root=True, + privsep_exec=False), mock.ANY, mock.call(['ebtables', '-t', 'nat', '--concurrent', '-A', 'PREROUTING', '-i', vif, '-j', mac_chain], check_exit_code=True, extra_ok_codes=None, - log_fail_as_error=True, run_as_root=True), + log_fail_as_error=True, run_as_root=True, + privsep_exec=False), mock.call(['ebtables', '-t', 'nat', '--concurrent', '-A', mac_chain, '-i', vif, '--among-src', '%s' % ','.join(sorted(mac_addresses)), '-j', 'RETURN'], check_exit_code=True, extra_ok_codes=None, - log_fail_as_error=True, run_as_root=True), + log_fail_as_error=True, run_as_root=True, + privsep_exec=False), mock.ANY, mock.ANY, mock.call(['ebtables', '-t', 'nat', '--concurrent', '-N', spoof_chain, '-P', 'DROP'], check_exit_code=True, extra_ok_codes=None, - log_fail_as_error=True, run_as_root=True), + log_fail_as_error=True, run_as_root=True, + privsep_exec=False), mock.call(['ebtables', '-t', 'nat', '--concurrent', '-F', spoof_chain], check_exit_code=True, extra_ok_codes=None, - log_fail_as_error=True, run_as_root=True), + log_fail_as_error=True, run_as_root=True, + privsep_exec=False), ] for addr in sorted(ip_addresses): expected.extend([ @@ -101,7 +107,8 @@ class TestLinuxBridgeARPSpoofing(base.BaseTestCase): spoof_chain, '-p', 'ARP', '--arp-ip-src', addr, '-j', 'ACCEPT'], check_exit_code=True, extra_ok_codes=None, - log_fail_as_error=True, run_as_root=True), + log_fail_as_error=True, run_as_root=True, + privsep_exec=False), ]) expected.extend([ mock.ANY, @@ -109,7 +116,8 @@ class TestLinuxBridgeARPSpoofing(base.BaseTestCase): 'PREROUTING', '-i', vif, '-j', spoof_chain, '-p', 'ARP'], check_exit_code=True, extra_ok_codes=None, - log_fail_as_error=True, run_as_root=True), + log_fail_as_error=True, run_as_root=True, + privsep_exec=False), ]) arp_protect.setup_arp_spoofing_protection(vif, port) @@ -129,56 +137,68 @@ class TestLinuxBridgeARPSpoofing(base.BaseTestCase): expected = [ mock.call(['ebtables', '-t', 'nat', '--concurrent', '-L'], check_exit_code=True, extra_ok_codes=None, - log_fail_as_error=True, run_as_root=True), + log_fail_as_error=True, run_as_root=True, + privsep_exec=False), mock.ANY, mock.call(['ebtables', '-t', 'nat', '--concurrent', '-D', 'PREROUTING', '-i', VIF, '-j', spoof_chain, '-p', 'ARP'], check_exit_code=True, extra_ok_codes=None, - log_fail_as_error=True, run_as_root=True), + log_fail_as_error=True, run_as_root=True, + privsep_exec=False), mock.call(['ebtables', '-t', 'nat', '--concurrent', '-F', spoof_chain], check_exit_code=True, extra_ok_codes=None, - log_fail_as_error=True, run_as_root=True), + log_fail_as_error=True, run_as_root=True, + privsep_exec=False), mock.call(['ebtables', '-t', 'nat', '--concurrent', '-X', spoof_chain], check_exit_code=True, extra_ok_codes=None, - log_fail_as_error=True, run_as_root=True), + log_fail_as_error=True, run_as_root=True, + privsep_exec=False), mock.ANY, mock.call(['ebtables', '-t', 'nat', '--concurrent', '-F', mac_chain], check_exit_code=True, extra_ok_codes=None, - log_fail_as_error=True, run_as_root=True), + log_fail_as_error=True, run_as_root=True, + privsep_exec=False), mock.call(['ebtables', '-t', 'nat', '--concurrent', '-X', mac_chain], check_exit_code=True, extra_ok_codes=None, - log_fail_as_error=True, run_as_root=True), + log_fail_as_error=True, run_as_root=True, + privsep_exec=False), mock.call(['ebtables', '-t', 'filter', '--concurrent', '-L'], check_exit_code=True, extra_ok_codes=None, - log_fail_as_error=True, run_as_root=True), + log_fail_as_error=True, run_as_root=True, + privsep_exec=False), mock.ANY, mock.call(['ebtables', '-t', 'filter', '--concurrent', '-D', 'FORWARD', '-i', VIF, '-j', spoof_chain, '-p', 'ARP'], check_exit_code=True, extra_ok_codes=None, - log_fail_as_error=True, run_as_root=True), + log_fail_as_error=True, run_as_root=True, + privsep_exec=False), mock.call(['ebtables', '-t', 'filter', '--concurrent', '-F', spoof_chain], check_exit_code=True, extra_ok_codes=None, - log_fail_as_error=True, run_as_root=True), + log_fail_as_error=True, run_as_root=True, + privsep_exec=False), mock.call(['ebtables', '-t', 'filter', '--concurrent', '-X', spoof_chain], check_exit_code=True, extra_ok_codes=None, - log_fail_as_error=True, run_as_root=True), + log_fail_as_error=True, run_as_root=True, + privsep_exec=False), mock.ANY, mock.call(['ebtables', '-t', 'filter', '--concurrent', '-F', mac_chain], check_exit_code=True, extra_ok_codes=None, - log_fail_as_error=True, run_as_root=True), + log_fail_as_error=True, run_as_root=True, + privsep_exec=False), mock.call(['ebtables', '-t', 'filter', '--concurrent', '-X', mac_chain], check_exit_code=True, extra_ok_codes=None, - log_fail_as_error=True, run_as_root=True), + log_fail_as_error=True, run_as_root=True, + privsep_exec=False), ] arp_protect.delete_arp_spoofing_protection([VIF]) diff --git a/releasenotes/notes/replace-rootwrap-with-privsep-5b85f1ba83df9554.yaml b/releasenotes/notes/replace-rootwrap-with-privsep-5b85f1ba83df9554.yaml new file mode 100644 index 00000000000..42835163283 --- /dev/null +++ b/releasenotes/notes/replace-rootwrap-with-privsep-5b85f1ba83df9554.yaml @@ -0,0 +1,17 @@ +--- +other: + - | + As defined in `Migrate from oslo.rootwrap to oslo.privsep + `_, + all OpenStack proyects should migrate from oslo.rootwrap to oslo.privsep + because "oslo.privsep offers a superior security model, faster and more + secure". + This migration will end with the deprecation and removal of oslo.rootwrap + from Neutron. To ensure the quality of the Neutron code, this migration + will be done sequentially in several patches, checking none of them breaks + the current functionality. + In order to easily migrate to execute all external commands inside a + privsep context, a new input variable "privsep_exec", that defaults to + "False", is added to ``neutron.agent.linux.utils.execute``. That will + divert the code to a privsep decorated executor. + Once the migration finishes, this new input parameter will be removed.