openstack/neutron
Code Issues Proposed changes

Do not run neutron-ns-metadata-proxy as root on L3 agent

Browse Source 
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
This commit is contained in:
Cedric Brandily 2014-11-24 15:53:04 +00:00
parent b99ba2716f
commit b78c5e54ab
8 changed files with 241 additions and 13 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

8
etc/l3_agent.ini
View File

@ -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

20
neutron/agent/l3/agent.py
View File

@ -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))

68
neutron/agent/linux/daemon.py
View File

@ -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)

22
neutron/agent/metadata/namespace_proxy.py
View File

@ -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()

5
neutron/common/exceptions.py
View File

@ -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

30
neutron/tests/unit/test_l3_agent.py
View File

@ -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)

93
neutron/tests/unit/test_linux_daemon.py
View File

@ -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()

8
neutron/tests/unit/test_metadata_namespace_proxy.py
View File

@ -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()]
)