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

diff --git a/etc/dhcp_agent.ini b/etc/dhcp_agent.ini
index 0f998789449b77c76a3e726b204335efc0578550..ea13ba6c0ceb1998b0ab58135dfc2e1c785b16b6 100644 (file)
--- 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 6e467a66de922f8e9e73035a144a16da16d95bda..1cf7baae4252c69f38a00ef01e0ded1872afba24 100644 (file)
--- 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)
-
-        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)
+                kwargs = {'router_id': router_ports[0].device_id}
+
+        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 134ae7280887a2f5380d44b89dbce005cec26616..b8bb8c136495354e7315386d1df6375a29c99ba4 100644 (file)
--- 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 1d3d96e4025f3b3110d0d5d16241f9f696b146ae..afb71da6e65d700f66f5879c650fc9895ca24d80 100644 (file)
--- 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 10314d7571b38752c3d84c63af903d39884a7cd7..2e9b52ab8b28e8ae4467563f857edd410a483cef 100644 (file)
--- 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 130a2ce34fbee8029cbd3c3b11c1e8c9ea9684e5..7f558caa9d6c81ca59a6dd94e848fd598fad7d86 100644 (file)
--- 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 3bf465acbaa8b55828d4bd040cf4e99aff2a0ea0..690453097b345a1322b1244344ddf295344b9a80 100644 (file)
--- 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 03f4e077710233605008c078d6130b0e860e74e3..35829ce878e5dee9e11cf70d7fdda2473fb105a2 100644 (file)
--- 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 568ac0a9a12f83e4e821d011e744f1333045f08b..987c3236c69b4df14335c6325237665eaf0db3ac 100644 (file)
--- 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 5e100e188b9b1f49b46cd36b3f1627e483b75030..793713b19cf6ac9333001dba13ba18eac9909c09 100644 (file)
--- 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 e64a1ad6cd2670c5caaafba328b2102c9da82c8a..d998a2c62bd33e457c4ddfc9a84988c40f848d83 100644 (file)
--- 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 928085ffad46579f8f16e0397b0eff50634e5191..4a707a503c6c3c389bd8f8b09c40c205042b564d 100644 (file)
--- 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):