From ac6cf685176c3a985a71174b9e8f0161068e38e0 Mon Sep 17 00:00:00 2001 From: Cedric Brandily Date: Wed, 7 Jan 2015 23:12:20 +0000 Subject: [PATCH] Do not run neutron-ns-metadata-proxy as root on dhcp agent Currently neutron-ns-metadata-proxy runs with root permissions when namespaces are enabled on the dhcp 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-dhcp-agent effective user and group. This change delegates metadata proxy management to metadata driver methods in order to reuse the work already done on l3 agent side. Permissions drop is done after metadata proxy daemon writes its pid in its pidfile (it could be disallowed after permissions drop) and after metadata proxy daemon binds its privileged server port (80). 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 Closes-Bug: #1187107 Change-Id: I53e97254d560e608101010f67bd2dcdec81fb6a2 --- etc/dhcp_agent.ini | 8 +++ neutron/agent/dhcp/agent.py | 29 +++------- neutron/agent/dhcp/config.py | 6 +- neutron/agent/dhcp_agent.py | 2 + neutron/agent/l3/ha_router.py | 3 +- neutron/agent/metadata/driver.py | 58 ++++++++++++------- neutron/agent/metadata/namespace_proxy.py | 9 ++- neutron/common/exceptions.py | 4 ++ .../tests/unit/agent/metadata/test_driver.py | 4 +- neutron/tests/unit/test_dhcp_agent.py | 38 +++++------- neutron/tests/unit/test_l3_agent.py | 16 +++-- .../unit/test_metadata_namespace_proxy.py | 4 +- 12 files changed, 101 insertions(+), 80 deletions(-) diff --git a/etc/dhcp_agent.ini b/etc/dhcp_agent.ini index 0f998789449..ea13ba6c0ce 100644 --- a/etc/dhcp_agent.ini +++ b/etc/dhcp_agent.ini @@ -75,6 +75,14 @@ # Use broadcast in DHCP replies # dhcp_broadcast_reply = False +# User (uid or name) running metadata proxy after its initialization +# (if empty: dhcp agent effective user) +# metadata_proxy_user = + +# Group (gid or name) running metadata proxy after its initialization +# (if empty: dhcp agent effective group) +# metadata_proxy_group = + # Location of Metadata Proxy UNIX domain socket # metadata_proxy_socket = $state_path/metadata_proxy diff --git a/neutron/agent/dhcp/agent.py b/neutron/agent/dhcp/agent.py index 6e467a66de9..1cf7baae425 100644 --- a/neutron/agent/dhcp/agent.py +++ b/neutron/agent/dhcp/agent.py @@ -22,9 +22,9 @@ from oslo_config import cfg import oslo_messaging from oslo_utils import importutils -from neutron.agent.common import config from neutron.agent.linux import dhcp from neutron.agent.linux import external_process +from neutron.agent.metadata import driver as metadata_driver from neutron.agent import rpc as agent_rpc from neutron.common import constants from neutron.common import exceptions @@ -336,7 +336,7 @@ class DhcpAgent(manager.Manager): # The proxy might work for either a single network # or all the networks connected via a router # to the one passed as a parameter - neutron_lookup_param = '--network_id=%s' % network.id + kwargs = {'network_id': network.id} # When the metadata network is enabled, the proxy might # be started for the router attached to the network if self.conf.enable_metadata_network: @@ -353,28 +353,15 @@ class DhcpAgent(manager.Manager): {'port_num': len(router_ports), 'port_id': router_ports[0].id, 'router_id': router_ports[0].device_id}) - neutron_lookup_param = ('--router_id=%s' % - router_ports[0].device_id) + kwargs = {'router_id': router_ports[0].device_id} - def callback(pid_file): - metadata_proxy_socket = cfg.CONF.metadata_proxy_socket - proxy_cmd = ['neutron-ns-metadata-proxy', - '--pid_file=%s' % pid_file, - '--metadata_proxy_socket=%s' % metadata_proxy_socket, - neutron_lookup_param, - '--state_path=%s' % self.conf.state_path, - '--metadata_port=%d' % dhcp.METADATA_PORT] - proxy_cmd.extend(config.get_log_args( - cfg.CONF, 'neutron-ns-metadata-proxy-%s.log' % network.id)) - return proxy_cmd - - self._process_monitor.enable(uuid=network.id, - cmd_callback=callback, - namespace=network.namespace) + metadata_driver.MetadataDriver.spawn_monitored_metadata_proxy( + self._process_monitor, network.namespace, dhcp.METADATA_PORT, + self.conf, **kwargs) def disable_isolated_metadata_proxy(self, network): - self._process_monitor.disable(uuid=network.id, - namespace=network.namespace) + metadata_driver.MetadataDriver.destroy_monitored_metadata_proxy( + self._process_monitor, network.id, network.namespace) class DhcpPluginApi(object): diff --git a/neutron/agent/dhcp/config.py b/neutron/agent/dhcp/config.py index 134ae728088..b8bb8c13649 100644 --- a/neutron/agent/dhcp/config.py +++ b/neutron/agent/dhcp/config.py @@ -29,11 +29,7 @@ DHCP_AGENT_OPTS = [ "dedicated network. Requires " "enable_isolated_metadata = True")), cfg.IntOpt('num_sync_threads', default=4, - help=_('Number of threads to use during sync process.')), - cfg.StrOpt('metadata_proxy_socket', - default='$state_path/metadata_proxy', - help=_('Location of Metadata Proxy UNIX domain ' - 'socket')), + help=_('Number of threads to use during sync process.')) ] DHCP_OPTS = [ diff --git a/neutron/agent/dhcp_agent.py b/neutron/agent/dhcp_agent.py index 1d3d96e4025..afb71da6e65 100644 --- a/neutron/agent/dhcp_agent.py +++ b/neutron/agent/dhcp_agent.py @@ -21,6 +21,7 @@ from oslo_config import cfg from neutron.agent.common import config from neutron.agent.dhcp import config as dhcp_config from neutron.agent.linux import interface +from neutron.agent.metadata import driver as metadata_driver from neutron.common import config as common_config from neutron.common import topics from neutron.openstack.common import service @@ -34,6 +35,7 @@ def register_options(): cfg.CONF.register_opts(dhcp_config.DHCP_AGENT_OPTS) cfg.CONF.register_opts(dhcp_config.DHCP_OPTS) cfg.CONF.register_opts(dhcp_config.DNSMASQ_OPTS) + cfg.CONF.register_opts(metadata_driver.MetadataDriver.OPTS) cfg.CONF.register_opts(interface.OPTS) diff --git a/neutron/agent/l3/ha_router.py b/neutron/agent/l3/ha_router.py index 10314d7571b..2e9b52ab8b2 100644 --- a/neutron/agent/l3/ha_router.py +++ b/neutron/agent/l3/ha_router.py @@ -110,7 +110,8 @@ class HaRouter(router.RouterInfo): def _add_keepalived_notifiers(self): callback = ( metadata_driver.MetadataDriver._get_metadata_proxy_callback( - self.router_id, self.agent_conf)) + self.agent_conf.metadata_port, self.agent_conf, + router_id=self.router_id)) # TODO(mangelajo): use the process monitor in keepalived when # keepalived stops killing/starting metadata # proxy on its own diff --git a/neutron/agent/metadata/driver.py b/neutron/agent/metadata/driver.py index 130a2ce34fb..7f558caa9d6 100644 --- a/neutron/agent/metadata/driver.py +++ b/neutron/agent/metadata/driver.py @@ -19,6 +19,7 @@ from oslo_config import cfg from neutron.agent.common import config from neutron.agent.linux import external_process +from neutron.common import exceptions from neutron.openstack.common import log as logging from neutron.services import advanced_service @@ -38,12 +39,12 @@ class MetadataDriver(advanced_service.AdvancedService): cfg.StrOpt('metadata_proxy_user', default='', help=_("User (uid or name) running metadata proxy after " - "its initialization (if empty: L3 agent effective " + "its initialization (if empty: 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 " + "its initialization (if empty: agent effective " "group)")) ] @@ -63,8 +64,12 @@ class MetadataDriver(advanced_service.AdvancedService): router.iptables_manager.apply() if not router.is_ha: - self._spawn_monitored_metadata_proxy(router.router_id, - router.ns_name) + self.spawn_monitored_metadata_proxy( + self.l3_agent.process_monitor, + router.ns_name, + self.metadata_port, + self.l3_agent.conf, + router_id=router.router_id) def before_router_removed(self, router): for c, r in self.metadata_filter_rules(self.metadata_port, @@ -76,8 +81,9 @@ class MetadataDriver(advanced_service.AdvancedService): router.iptables_manager.ipv4['nat'].remove_rule(c, r) router.iptables_manager.apply() - self._destroy_monitored_metadata_proxy(router.router['id'], - router.ns_name) + self.destroy_monitored_metadata_proxy(self.l3_agent.process_monitor, + router.router['id'], + router.ns_name) @classmethod def metadata_filter_rules(cls, port, mark): @@ -106,7 +112,16 @@ class MetadataDriver(advanced_service.AdvancedService): return user, group @classmethod - def _get_metadata_proxy_callback(cls, router_id, conf): + def _get_metadata_proxy_callback(cls, port, conf, network_id=None, + router_id=None): + uuid = network_id or router_id + if uuid is None: + raise exceptions.NetworkIdOrRouterIdRequiredError() + + if network_id: + lookup_param = '--network_id=%s' % network_id + else: + lookup_param = '--router_id=%s' % router_id def callback(pid_file): metadata_proxy_socket = conf.metadata_proxy_socket @@ -114,25 +129,27 @@ class MetadataDriver(advanced_service.AdvancedService): proxy_cmd = ['neutron-ns-metadata-proxy', '--pid_file=%s' % pid_file, '--metadata_proxy_socket=%s' % metadata_proxy_socket, - '--router_id=%s' % router_id, + lookup_param, '--state_path=%s' % conf.state_path, - '--metadata_port=%s' % conf.metadata_port, + '--metadata_port=%s' % port, '--metadata_proxy_user=%s' % user, '--metadata_proxy_group=%s' % group] proxy_cmd.extend(config.get_log_args( - conf, 'neutron-ns-metadata-proxy-%s.log' % - router_id)) + conf, 'neutron-ns-metadata-proxy-%s.log' % uuid)) return proxy_cmd return callback - def _spawn_monitored_metadata_proxy(self, router_id, ns_name): - callback = self._get_metadata_proxy_callback( - router_id, self.l3_agent.conf) - self.l3_agent.process_monitor.enable(router_id, callback, ns_name) + @classmethod + def spawn_monitored_metadata_proxy(cls, monitor, ns_name, port, conf, + network_id=None, router_id=None): + callback = cls._get_metadata_proxy_callback( + port, conf, network_id=network_id, router_id=router_id) + monitor.enable(network_id or router_id, callback, ns_name) - def _destroy_monitored_metadata_proxy(self, router_id, ns_name): - self.l3_agent.process_monitor.disable(router_id, ns_name) + @classmethod + def destroy_monitored_metadata_proxy(cls, monitor, uuid, ns_name): + monitor.disable(uuid, ns_name) # TODO(mangelajo): remove the unmonitored _get_*_process_manager, # _spawn_* and _destroy* when keepalived stops @@ -145,12 +162,13 @@ class MetadataDriver(advanced_service.AdvancedService): ns_name) @classmethod - def _spawn_metadata_proxy(cls, router_id, ns_name, conf): - callback = cls._get_metadata_proxy_callback(router_id, conf) + def spawn_metadata_proxy(cls, router_id, ns_name, port, conf): + callback = cls._get_metadata_proxy_callback(port, conf, + router_id=router_id) pm = cls._get_metadata_proxy_process_manager(router_id, ns_name, conf) pm.enable(callback) @classmethod - def _destroy_metadata_proxy(cls, router_id, ns_name, conf): + def destroy_metadata_proxy(cls, router_id, ns_name, conf): pm = cls._get_metadata_proxy_process_manager(router_id, ns_name, conf) pm.disable() diff --git a/neutron/agent/metadata/namespace_proxy.py b/neutron/agent/metadata/namespace_proxy.py index 3bf465acbaa..690453097b3 100644 --- a/neutron/agent/metadata/namespace_proxy.py +++ b/neutron/agent/metadata/namespace_proxy.py @@ -22,6 +22,7 @@ import webob from neutron.agent.linux import daemon from neutron.common import config +from neutron.common import exceptions from neutron.common import utils from neutron.i18n import _LE from neutron.openstack.common import log as logging @@ -56,8 +57,7 @@ class NetworkMetadataProxyHandler(object): self.router_id = router_id if network_id is None and router_id is None: - msg = _('network_id and router_id are None. One must be provided.') - raise ValueError(msg) + raise exceptions.NetworkIdOrRouterIdRequiredError() @webob.dec.wsgify(RequestClass=webob.Request) def __call__(self, req): @@ -135,12 +135,15 @@ class ProxyDaemon(daemon.Daemon): self.port = port def run(self): - super(ProxyDaemon, self).run() handler = NetworkMetadataProxyHandler( self.network_id, self.router_id) proxy = wsgi.Server('neutron-network-metadata-proxy') proxy.start(handler, self.port) + + # Drop privileges after port bind + super(ProxyDaemon, self).run() + proxy.wait() diff --git a/neutron/common/exceptions.py b/neutron/common/exceptions.py index 03f4e077710..35829ce878e 100644 --- a/neutron/common/exceptions.py +++ b/neutron/common/exceptions.py @@ -369,6 +369,10 @@ class IpTablesApplyException(NeutronException): super(IpTablesApplyException, self).__init__() +class NetworkIdOrRouterIdRequiredError(NeutronException): + message = _('network_id and router_id are None. One must be provided.') + + # Shared *aas exceptions, pending them being refactored out of Neutron # proper. diff --git a/neutron/tests/unit/agent/metadata/test_driver.py b/neutron/tests/unit/agent/metadata/test_driver.py index 568ac0a9a12..987c3236c69 100644 --- a/neutron/tests/unit/agent/metadata/test_driver.py +++ b/neutron/tests/unit/agent/metadata/test_driver.py @@ -68,7 +68,6 @@ class TestMetadataDriver(base.BaseTestCase): metadata_port = 8080 ip_class_path = 'neutron.agent.linux.ip_lib.IPWrapper' - cfg.CONF.set_override('metadata_port', metadata_port) cfg.CONF.set_override('metadata_proxy_user', user) cfg.CONF.set_override('metadata_proxy_group', group) cfg.CONF.set_override('log_file', 'test.log') @@ -79,7 +78,8 @@ class TestMetadataDriver(base.BaseTestCase): 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): - driver._spawn_metadata_proxy(router_id, router_ns, cfg.CONF) + driver.spawn_metadata_proxy(router_id, router_ns, metadata_port, + cfg.CONF) ip_mock.assert_has_calls([ mock.call(namespace=router_ns), mock.call().netns.execute([ diff --git a/neutron/tests/unit/test_dhcp_agent.py b/neutron/tests/unit/test_dhcp_agent.py index 5e100e188b9..793713b19cf 100644 --- a/neutron/tests/unit/test_dhcp_agent.py +++ b/neutron/tests/unit/test_dhcp_agent.py @@ -751,34 +751,28 @@ class TestDhcpAgentEventHandler(base.BaseTestCase): ]) def test_disable_isolated_metadata_proxy(self): - self.dhcp.disable_isolated_metadata_proxy(fake_network) - self.external_process.assert_has_calls([ - self._process_manager_constructor_call(), - mock.call().disable() - ]) + method_path = ('neutron.agent.metadata.driver.MetadataDriver' + '.destroy_monitored_metadata_proxy') + with mock.patch(method_path) as destroy: + self.dhcp.disable_isolated_metadata_proxy(fake_network) + destroy.assert_called_once_with(self.dhcp._process_monitor, + fake_network.id, + fake_network.namespace) def _test_metadata_network(self, network): cfg.CONF.set_override('enable_metadata_network', True) cfg.CONF.set_override('debug', True) cfg.CONF.set_override('verbose', False) cfg.CONF.set_override('log_file', 'test.log') - self.dhcp.enable_isolated_metadata_proxy(network) - - self.external_process.assert_has_calls([ - self._process_manager_constructor_call()]) - - callback = self.external_process.call_args[1]['default_cmd_callback'] - result_cmd = callback('pidfile') - self.assertEqual( - result_cmd, - ['neutron-ns-metadata-proxy', - '--pid_file=pidfile', - '--metadata_proxy_socket=%s' % cfg.CONF.metadata_proxy_socket, - '--router_id=forzanapoli', - '--state_path=%s' % cfg.CONF.state_path, - '--metadata_port=%d' % dhcp.METADATA_PORT, - '--debug', - '--log-file=neutron-ns-metadata-proxy-%s.log' % network.id]) + method_path = ('neutron.agent.metadata.driver.MetadataDriver' + '.spawn_monitored_metadata_proxy') + with mock.patch(method_path) as spawn: + self.dhcp.enable_isolated_metadata_proxy(network) + spawn.assert_called_once_with(self.dhcp._process_monitor, + network.namespace, + dhcp.METADATA_PORT, + cfg.CONF, + router_id='forzanapoli') def test_enable_isolated_metadata_proxy_with_metadata_network(self): self._test_metadata_network(fake_meta_network) diff --git a/neutron/tests/unit/test_l3_agent.py b/neutron/tests/unit/test_l3_agent.py index e64a1ad6cd2..d998a2c62bd 100644 --- a/neutron/tests/unit/test_l3_agent.py +++ b/neutron/tests/unit/test_l3_agent.py @@ -1608,18 +1608,24 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework): 'distributed': False} driver = metadata_driver.MetadataDriver with mock.patch.object( - driver, '_destroy_monitored_metadata_proxy') as destroy_proxy: + driver, 'destroy_monitored_metadata_proxy') as destroy_proxy: with mock.patch.object( - driver, '_spawn_monitored_metadata_proxy') as spawn_proxy: + driver, 'spawn_monitored_metadata_proxy') as spawn_proxy: agent._process_added_router(router) if enableflag: - spawn_proxy.assert_called_with(router_id, - mock.ANY) + spawn_proxy.assert_called_with( + mock.ANY, + mock.ANY, + self.conf.metadata_port, + mock.ANY, + router_id=router_id + ) else: self.assertFalse(spawn_proxy.call_count) agent._router_removed(router_id) if enableflag: - destroy_proxy.assert_called_with(router_id, + destroy_proxy.assert_called_with(mock.ANY, + router_id, mock.ANY) else: self.assertFalse(destroy_proxy.call_count) diff --git a/neutron/tests/unit/test_metadata_namespace_proxy.py b/neutron/tests/unit/test_metadata_namespace_proxy.py index 928085ffad4..4a707a503c6 100644 --- a/neutron/tests/unit/test_metadata_namespace_proxy.py +++ b/neutron/tests/unit/test_metadata_namespace_proxy.py @@ -19,6 +19,7 @@ import testtools import webob from neutron.agent.metadata import namespace_proxy as ns_proxy +from neutron.common import exceptions from neutron.common import utils from neutron.tests import base @@ -75,7 +76,8 @@ class TestNetworkMetadataProxyHandler(base.BaseTestCase): req.body) def test_no_argument_passed_to_init(self): - with testtools.ExpectedException(ValueError): + with testtools.ExpectedException( + exceptions.NetworkIdOrRouterIdRequiredError): ns_proxy.NetworkMetadataProxyHandler() def test_call_internal_server_error(self):