From: Cedric Brandily Date: Mon, 24 Nov 2014 15:53:04 +0000 (+0000) Subject: Do not run neutron-ns-metadata-proxy as root on L3 agent X-Git-Url: https://review.fuel-infra.org/gitweb?a=commitdiff_plain;h=b78c5e54abd10fc71a46788110f9f36e6496414e;p=openstack-build%2Fneutron-build.git Do not run neutron-ns-metadata-proxy as root on L3 agent Currently neutron-ns-metadata-proxy runs with root permissions when namespaces are enabled on the l3 agent because root permissions are required to "enter" in the namespace. But neutron-ns-metadata-proxy permissions should be reduced as much as possible because it is reachable from vms. This change allows to change neutron-ns-metadata-proxy permissions after its startup through the 2 new options metadata_proxy_user and metadata_proxy_group which allow to define user/group running metadata proxy after its initialization. Their default values are neutron-l3-agent effective user and group. Permissions drop is done after metadata proxy daemon writes its pid in its pidfile (it could be disallowed after permissions drop). Using nobody as metadata_proxy_user/group (more secure) is currently not supported because: * nobody has not the permission to connect the metadata socket, * nobody has not the permission to log to file because neutron uses WatchedFileHandler (which requires read/write permissions after permissions drop). This limitation will be addressed in a daughter change. DocImpact Partial-Bug: #1187107 Change-Id: I55c8c3fb14ed91ae8570f98f19c2cdbaf89d42fc --- diff --git a/etc/l3_agent.ini b/etc/l3_agent.ini index 94c971475..0c158370d 100644 --- a/etc/l3_agent.ini +++ b/etc/l3_agent.ini @@ -48,6 +48,14 @@ # TCP Port used by Neutron metadata server # metadata_port = 9697 +# User (uid or name) running metadata proxy after its initialization +# (if empty: L3 agent effective user) +# metadata_proxy_user = + +# Group (gid or name) running metadata proxy after its initialization +# (if empty: L3 agent effective group) +# metadata_proxy_group = + # Send this many gratuitous ARPs for HA setup. Set it below or equal to 0 # to disable this feature. # send_arp_for_ha = 3 diff --git a/neutron/agent/l3/agent.py b/neutron/agent/l3/agent.py index 98953e526..4d048925d 100644 --- a/neutron/agent/l3/agent.py +++ b/neutron/agent/l3/agent.py @@ -202,6 +202,16 @@ class L3NATAgent(firewall_l3_agent.FWaaSL3AgentRpcCallback, default='$state_path/metadata_proxy', help=_('Location of Metadata Proxy UNIX domain ' 'socket')), + cfg.StrOpt('metadata_proxy_user', + default='', + help=_("User (uid or name) running metadata proxy after " + "its initialization (if empty: L3 agent effective " + "user)")), + cfg.StrOpt('metadata_proxy_group', + default='', + help=_("Group (gid or name) running metadata proxy after " + "its initialization (if empty: L3 agent effective " + "group)")) ] def __init__(self, host, conf=None): @@ -512,16 +522,24 @@ class L3NATAgent(firewall_l3_agent.FWaaSL3AgentRpcCallback, self.event_observers.notify( adv_svc.AdvancedService.after_router_removed, ri) + def _get_metadata_proxy_user_group(self): + user = self.conf.metadata_proxy_user or os.geteuid() + group = self.conf.metadata_proxy_group or os.getegid() + return user, group + def _get_metadata_proxy_callback(self, router_id): def callback(pid_file): metadata_proxy_socket = self.conf.metadata_proxy_socket + user, group = self._get_metadata_proxy_user_group() proxy_cmd = ['neutron-ns-metadata-proxy', '--pid_file=%s' % pid_file, '--metadata_proxy_socket=%s' % metadata_proxy_socket, '--router_id=%s' % router_id, '--state_path=%s' % self.conf.state_path, - '--metadata_port=%s' % self.conf.metadata_port] + '--metadata_port=%s' % self.conf.metadata_port, + '--metadata_proxy_user=%s' % user, + '--metadata_proxy_group=%s' % group] proxy_cmd.extend(config.get_log_args( self.conf, 'neutron-ns-metadata-proxy-%s.log' % router_id)) diff --git a/neutron/agent/linux/daemon.py b/neutron/agent/linux/daemon.py index 047a8ee03..42ace422a 100644 --- a/neutron/agent/linux/daemon.py +++ b/neutron/agent/linux/daemon.py @@ -14,16 +14,73 @@ import atexit import fcntl +import grp import os +import pwd import signal import sys -from neutron.i18n import _LE +from neutron.common import exceptions +from neutron.i18n import _LE, _LI from neutron.openstack.common import log as logging LOG = logging.getLogger(__name__) +def setuid(user_id_or_name): + try: + new_uid = int(user_id_or_name) + except (TypeError, ValueError): + new_uid = pwd.getpwnam(user_id_or_name).pw_uid + if new_uid != 0: + try: + os.setuid(new_uid) + except OSError: + msg = _('Failed to set uid %s') % new_uid + LOG.critical(msg) + raise exceptions.FailToDropPrivilegesExit(msg) + + +def setgid(group_id_or_name): + try: + new_gid = int(group_id_or_name) + except (TypeError, ValueError): + new_gid = grp.getgrnam(group_id_or_name).gr_gid + if new_gid != 0: + try: + os.setgid(new_gid) + except OSError: + msg = _('Failed to set gid %s') % new_gid + LOG.critical(msg) + raise exceptions.FailToDropPrivilegesExit(msg) + + +def drop_privileges(user=None, group=None): + """Drop privileges to user/group privileges.""" + if user is None and group is None: + return + + if os.geteuid() != 0: + msg = _('Root permissions are required to drop privileges.') + LOG.critical(msg) + raise exceptions.FailToDropPrivilegesExit(msg) + + if group is not None: + try: + os.setgroups([]) + except OSError: + msg = _('Failed to remove supplemental groups') + LOG.critical(msg) + raise exceptions.FailToDropPrivilegesExit(msg) + setgid(group) + + if user is not None: + setuid(user) + + LOG.info(_LI("Process runs with uid/gid: %(uid)s/%(gid)s"), + {'uid': os.getuid(), 'gid': os.getgid()}) + + class Pidfile(object): def __init__(self, pidfile, procname, uuid=None): self.pidfile = pidfile @@ -77,12 +134,15 @@ class Daemon(object): Usage: subclass the Daemon class and override the run() method """ def __init__(self, pidfile, stdin='/dev/null', stdout='/dev/null', - stderr='/dev/null', procname='python', uuid=None): + stderr='/dev/null', procname='python', uuid=None, + user=None, group=None): self.stdin = stdin self.stdout = stdout self.stderr = stderr self.procname = procname self.pidfile = Pidfile(pidfile, procname, uuid) + self.user = user + self.group = group def _fork(self): try: @@ -141,8 +201,8 @@ class Daemon(object): self.run() def run(self): - """Override this method when subclassing Daemon. + """Override this method and call super().run when subclassing Daemon. start() will call this method after the process has daemonized. """ - pass + drop_privileges(self.user, self.group) diff --git a/neutron/agent/metadata/namespace_proxy.py b/neutron/agent/metadata/namespace_proxy.py index a48d5bbfd..8fb6c54bf 100644 --- a/neutron/agent/metadata/namespace_proxy.py +++ b/neutron/agent/metadata/namespace_proxy.py @@ -128,14 +128,17 @@ class NetworkMetadataProxyHandler(object): class ProxyDaemon(daemon.Daemon): - def __init__(self, pidfile, port, network_id=None, router_id=None): + def __init__(self, pidfile, port, network_id=None, router_id=None, + user=None, group=None): uuid = network_id or router_id - super(ProxyDaemon, self).__init__(pidfile, uuid=uuid) + super(ProxyDaemon, self).__init__(pidfile, uuid=uuid, user=user, + group=group) self.network_id = network_id self.router_id = router_id self.port = port def run(self): + super(ProxyDaemon, self).run() handler = NetworkMetadataProxyHandler( self.network_id, self.router_id) @@ -164,7 +167,15 @@ def main(): cfg.StrOpt('metadata_proxy_socket', default='$state_path/metadata_proxy', help=_('Location of Metadata Proxy UNIX domain ' - 'socket')) + 'socket')), + cfg.StrOpt('metadata_proxy_user', + default=None, + help=_("User (uid or name) running metadata proxy after " + "its initialization")), + cfg.StrOpt('metadata_proxy_group', + default=None, + help=_("Group (gid or name) running metadata proxy after " + "its initialization")), ] cfg.CONF.register_cli_opts(opts) @@ -172,10 +183,13 @@ def main(): cfg.CONF(project='neutron', default_config_files=[]) config.setup_logging() utils.log_opt_values(LOG) + proxy = ProxyDaemon(cfg.CONF.pid_file, cfg.CONF.metadata_port, network_id=cfg.CONF.network_id, - router_id=cfg.CONF.router_id) + router_id=cfg.CONF.router_id, + user=cfg.CONF.metadata_proxy_user, + group=cfg.CONF.metadata_proxy_group) if cfg.CONF.daemonize: proxy.start() diff --git a/neutron/common/exceptions.py b/neutron/common/exceptions.py index ccbe891ca..a629f26cd 100644 --- a/neutron/common/exceptions.py +++ b/neutron/common/exceptions.py @@ -339,3 +339,8 @@ class InvalidCIDR(BadRequest): class RouterNotCompatibleWithAgent(NeutronException): message = _("Router '%(router_id)s' is not compatible with this agent") + + +class FailToDropPrivilegesExit(SystemExit): + """Exit exception raised when a drop privileges action fails.""" + code = 99 diff --git a/neutron/tests/unit/test_l3_agent.py b/neutron/tests/unit/test_l3_agent.py index aac84b28c..2915f040e 100644 --- a/neutron/tests/unit/test_l3_agent.py +++ b/neutron/tests/unit/test_l3_agent.py @@ -2198,6 +2198,9 @@ vrrp_instance VR_1 { class TestL3AgentEventHandler(base.BaseTestCase): + EUID = '123' + EGID = '456' + def setUp(self): super(TestL3AgentEventHandler, self).setUp() cfg.CONF.register_opts(l3_agent.L3NATAgent.OPTS) @@ -2240,7 +2243,8 @@ class TestL3AgentEventHandler(base.BaseTestCase): looping_call_p.start() self.agent = l3_agent.L3NATAgent(HOSTNAME) - def test_spawn_metadata_proxy(self): + def _test_spawn_metadata_proxy(self, expected_user, expected_group, + user='', group=''): router_id = _uuid() metadata_port = 8080 ip_class_path = 'neutron.agent.linux.ip_lib.IPWrapper' @@ -2248,10 +2252,15 @@ class TestL3AgentEventHandler(base.BaseTestCase): cfg.CONF.set_override('metadata_port', metadata_port) cfg.CONF.set_override('log_file', 'test.log') cfg.CONF.set_override('debug', True) + cfg.CONF.set_override('metadata_proxy_user', user) + cfg.CONF.set_override('metadata_proxy_group', group) self.external_process_p.stop() ri = l3router.RouterInfo(router_id, None, None) - with mock.patch(ip_class_path) as ip_mock: + with contextlib.nested( + mock.patch('os.geteuid', return_value=self.EUID), + mock.patch('os.getegid', return_value=self.EGID), + mock.patch(ip_class_path)) as (geteuid, getegid, ip_mock): self.agent._spawn_metadata_proxy(ri.router_id, ri.ns_name) ip_mock.assert_has_calls([ mock.call('sudo', ri.ns_name), @@ -2262,8 +2271,25 @@ class TestL3AgentEventHandler(base.BaseTestCase): '--router_id=%s' % router_id, mock.ANY, '--metadata_port=%s' % metadata_port, + '--metadata_proxy_user=%s' % expected_user, + '--metadata_proxy_group=%s' % expected_group, '--debug', '--log-file=neutron-ns-metadata-proxy-%s.log' % router_id ], addl_env=None) ]) + + def test_spawn_metadata_proxy_with_user(self): + self._test_spawn_metadata_proxy('user', self.EGID, user='user') + + def test_spawn_metadata_proxy_with_uid(self): + self._test_spawn_metadata_proxy('321', self.EGID, user='321') + + def test_spawn_metadata_proxy_with_group(self): + self._test_spawn_metadata_proxy(self.EUID, 'group', group='group') + + def test_spawn_metadata_proxy_with_gid(self): + self._test_spawn_metadata_proxy(self.EUID, '654', group='654') + + def test_spawn_metadata_proxy(self): + self._test_spawn_metadata_proxy(self.EUID, self.EGID) diff --git a/neutron/tests/unit/test_linux_daemon.py b/neutron/tests/unit/test_linux_daemon.py index f5408961c..ce5644fd7 100644 --- a/neutron/tests/unit/test_linux_daemon.py +++ b/neutron/tests/unit/test_linux_daemon.py @@ -13,6 +13,8 @@ # License for the specific language governing permissions and limitations # under the License. + +import contextlib import os import sys @@ -20,11 +22,102 @@ import mock import testtools from neutron.agent.linux import daemon +from neutron.common import exceptions from neutron.tests import base FAKE_FD = 8 +class FakeEntry(object): + def __init__(self, name, value): + setattr(self, name, value) + + +class TestPrivileges(base.BaseTestCase): + def test_setuid_with_name(self): + with mock.patch('pwd.getpwnam', return_value=FakeEntry('pw_uid', 123)): + with mock.patch('os.setuid') as setuid_mock: + daemon.setuid('user') + setuid_mock.assert_called_once_with(123) + + def test_setuid_with_id(self): + with mock.patch('os.setuid') as setuid_mock: + daemon.setuid('321') + setuid_mock.assert_called_once_with(321) + + def test_setuid_fails(self): + with mock.patch('os.setuid', side_effect=OSError()): + with mock.patch.object(daemon.LOG, 'critical') as log_critical: + self.assertRaises(exceptions.FailToDropPrivilegesExit, + daemon.setuid, '321') + log_critical.assert_once_with(mock.ANY) + + def test_setgid_with_name(self): + with mock.patch('grp.getgrnam', return_value=FakeEntry('gr_gid', 123)): + with mock.patch('os.setgid') as setgid_mock: + daemon.setgid('group') + setgid_mock.assert_called_once_with(123) + + def test_setgid_with_id(self): + with mock.patch('os.setgid') as setgid_mock: + daemon.setgid('321') + setgid_mock.assert_called_once_with(321) + + def test_setgid_fails(self): + with mock.patch('os.setgid', side_effect=OSError()): + with mock.patch.object(daemon.LOG, 'critical') as log_critical: + self.assertRaises(exceptions.FailToDropPrivilegesExit, + daemon.setgid, '321') + log_critical.assert_once_with(mock.ANY) + + def test_drop_no_privileges(self): + with contextlib.nested( + mock.patch.object(os, 'setgroups'), + mock.patch.object(daemon, 'setgid'), + mock.patch.object(daemon, 'setuid')) as mocks: + daemon.drop_privileges() + for cursor in mocks: + self.assertFalse(cursor.called) + + def _test_drop_privileges(self, user=None, group=None): + with contextlib.nested( + mock.patch.object(os, 'geteuid', return_value=0), + mock.patch.object(os, 'setgroups'), + mock.patch.object(daemon, 'setgid'), + mock.patch.object(daemon, 'setuid')) as ( + geteuid, setgroups, setgid, setuid): + daemon.drop_privileges(user=user, group=group) + if user: + setuid.assert_called_once_with(user) + else: + self.assertFalse(setuid.called) + if group: + setgroups.assert_called_once_with([]) + setgid.assert_called_once_with(group) + else: + self.assertFalse(setgroups.called) + self.assertFalse(setgid.called) + + def test_drop_user_privileges(self): + self._test_drop_privileges(user='user') + + def test_drop_uid_privileges(self): + self._test_drop_privileges(user='321') + + def test_drop_group_privileges(self): + self._test_drop_privileges(group='group') + + def test_drop_gid_privileges(self): + self._test_drop_privileges(group='654') + + def test_drop_privileges_without_root_permissions(self): + with mock.patch('os.geteuid', return_value=1): + with mock.patch.object(daemon.LOG, 'critical') as log_critical: + self.assertRaises(exceptions.FailToDropPrivilegesExit, + daemon.drop_privileges, 'user') + log_critical.assert_once_with(mock.ANY) + + class TestPidfile(base.BaseTestCase): def setUp(self): super(TestPidfile, self).setUp() diff --git a/neutron/tests/unit/test_metadata_namespace_proxy.py b/neutron/tests/unit/test_metadata_namespace_proxy.py index 3ffbd9921..928085ffa 100644 --- a/neutron/tests/unit/test_metadata_namespace_proxy.py +++ b/neutron/tests/unit/test_metadata_namespace_proxy.py @@ -307,7 +307,9 @@ class TestProxyDaemon(base.BaseTestCase): daemon.assert_has_calls([ mock.call('pidfile', 9697, router_id='router_id', - network_id=None), + network_id=None, + user=mock.ANY, + group=mock.ANY), mock.call().start()] ) @@ -328,6 +330,8 @@ class TestProxyDaemon(base.BaseTestCase): daemon.assert_has_calls([ mock.call('pidfile', 9697, router_id='router_id', - network_id=None), + network_id=None, + user=mock.ANY, + group=mock.ANY), mock.call().run()] )