From: Cedric Brandily Date: Tue, 3 Mar 2015 22:26:52 +0000 (+0000) Subject: Allow metadata proxy to log with nobody user/group X-Git-Url: https://review.fuel-infra.org/gitweb?a=commitdiff_plain;h=fbc22784149cd6b3ca6d8161e360d3d7c10d94ac;p=openstack-build%2Fneutron-build.git Allow metadata proxy to log with nobody user/group Currently metadata proxy cannot run with nobody user/group as metadata proxy (as other services) uses WatchedFileHandler handler to log to file which does not support permissions drop (the process must be able to r/w after permissions drop to "watch" the file). This change allows to enable/disable log watch in metadata proxies with the new option metadata_proxy_log_watch. It should be disabled when metadata_proxy_user/group is not allowed to read/write metadata proxy log files. Option default value is deduced from metadata_proxy_user: * True if metadata_proxy_user is agent effective user id/name, * False otherwise. When log watch is disabled and logrotate is enabled on metadata proxy logging files, 'copytruncate' logrotate option must be used otherwise metadata proxy logs will be lost after the first log rotation. DocImpact Change-Id: I40a7bd82a2c60d9198312fdb52e3010c60db3511 Partial-Bug: #1427228 --- diff --git a/etc/neutron.conf b/etc/neutron.conf index e401f7688..e8dd9db81 100644 --- a/etc/neutron.conf +++ b/etc/neutron.conf @@ -239,6 +239,14 @@ lock_path = $state_path/lock # (if empty: agent effective group) # metadata_proxy_group = +# Enable/Disable log watch by metadata proxy, it should be disabled when +# metadata_proxy_user/group is not allowed to read/write its log file and +# 'copytruncate' logrotate option must be used if logrotate is enabled on +# metadata proxy log files. Option default value is deduced from +# metadata_proxy_user: watch log is enabled if metadata_proxy_user is agent +# effective user id/name. +# metadata_proxy_watch_log = + # Location of Metadata Proxy UNIX domain socket # metadata_proxy_socket = $state_path/metadata_proxy # =========== end of items for metadata proxy configuration ============== diff --git a/neutron/agent/common/config.py b/neutron/agent/common/config.py index 3f533fa3a..49e27b92d 100644 --- a/neutron/agent/common/config.py +++ b/neutron/agent/common/config.py @@ -73,7 +73,7 @@ PROCESS_MONITOR_OPTS = [ ] -def get_log_args(conf, log_file_name): +def get_log_args(conf, log_file_name, **kwargs): cmd_args = [] if conf.debug: cmd_args.append('--debug') @@ -91,6 +91,8 @@ def get_log_args(conf, log_file_name): log_dir = os.path.dirname(conf.log_file) if log_dir: cmd_args.append('--log-dir=%s' % log_dir) + if kwargs.get('metadata_proxy_watch_log') is False: + cmd_args.append('--metadata_proxy_watch_log=false') else: if conf.use_syslog: cmd_args.append('--use-syslog') diff --git a/neutron/agent/linux/daemon.py b/neutron/agent/linux/daemon.py index 65ea4af86..f9866445f 100644 --- a/neutron/agent/linux/daemon.py +++ b/neutron/agent/linux/daemon.py @@ -15,6 +15,8 @@ import atexit import fcntl import grp +import logging as std_logging +from logging import handlers import os import pwd import signal @@ -56,6 +58,25 @@ def setgid(group_id_or_name): raise exceptions.FailToDropPrivilegesExit(msg) +def unwatch_log(): + """Replace WatchedFileHandler handlers by FileHandler ones. + + Neutron logging uses WatchedFileHandler handlers but they do not + support privileges drop, this method replaces them by FileHandler + handlers supporting privileges drop. + """ + log_root = logging.getLogger(None).logger + to_replace = [h for h in log_root.handlers + if isinstance(h, handlers.WatchedFileHandler)] + for handler in to_replace: + new_handler = std_logging.FileHandler(handler.baseFilename, + mode=handler.mode, + encoding=handler.encoding, + delay=handler.delay) + log_root.removeHandler(handler) + log_root.addHandler(new_handler) + + def drop_privileges(user=None, group=None): """Drop privileges to user/group privileges.""" if user is None and group is None: @@ -136,7 +157,7 @@ class Daemon(object): """ def __init__(self, pidfile, stdin='/dev/null', stdout='/dev/null', stderr='/dev/null', procname='python', uuid=None, - user=None, group=None): + user=None, group=None, watch_log=True): self.stdin = stdin self.stdout = stdout self.stderr = stderr @@ -144,6 +165,7 @@ class Daemon(object): self.pidfile = Pidfile(pidfile, procname, uuid) self.user = user self.group = group + self.watch_log = watch_log def _fork(self): try: @@ -206,4 +228,6 @@ class Daemon(object): start() will call this method after the process has daemonized. """ + if not self.watch_log: + unwatch_log() drop_privileges(self.user, self.group) diff --git a/neutron/agent/linux/utils.py b/neutron/agent/linux/utils.py index c5aa4c7e0..49acd7afe 100644 --- a/neutron/agent/linux/utils.py +++ b/neutron/agent/linux/utils.py @@ -17,6 +17,7 @@ import fcntl import glob import httplib import os +import pwd import shlex import socket import struct @@ -334,6 +335,15 @@ def ensure_directory_exists_without_file(path): ensure_dir(dirname) +def is_effective_user(user_id_or_name): + """Returns True if user_id_or_name is effective user (id/name).""" + euid = os.geteuid() + if str(user_id_or_name) == str(euid): + return True + effective_user_name = pwd.getpwuid(euid).pw_name + return user_id_or_name == effective_user_name + + class UnixDomainHTTPConnection(httplib.HTTPConnection): """Connection class for HTTP over UNIX domain socket.""" def __init__(self, host, port=None, strict=None, timeout=None, diff --git a/neutron/agent/metadata/driver.py b/neutron/agent/metadata/driver.py index bdb01354c..9378a3a93 100644 --- a/neutron/agent/metadata/driver.py +++ b/neutron/agent/metadata/driver.py @@ -21,6 +21,7 @@ from oslo_log import log as logging from neutron.agent.common import config from neutron.agent.l3 import namespaces from neutron.agent.linux import external_process +from neutron.agent.linux import utils from neutron.common import exceptions from neutron.services import advanced_service @@ -47,7 +48,18 @@ class MetadataDriver(advanced_service.AdvancedService): default='', help=_("Group (gid or name) running metadata proxy after " "its initialization (if empty: agent effective " - "group)")) + "group)")), + cfg.BoolOpt('metadata_proxy_watch_log', + default=None, + help=_("Enable/Disable log watch by metadata proxy. It " + "should be disabled when metadata_proxy_user/group " + "is not allowed to read/write its log file and " + "copytruncate logrotate option must be used if " + "logrotate is enabled on metadata proxy log " + "files. Option default value is deduced from " + "metadata_proxy_user: watch log is enabled if " + "metadata_proxy_user is agent effective user " + "id/name.")), ] def __init__(self, l3_agent): @@ -112,10 +124,17 @@ class MetadataDriver(advanced_service.AdvancedService): 'port': port})] @classmethod - def _get_metadata_proxy_user_group(cls, conf): - user = conf.metadata_proxy_user or os.geteuid() - group = conf.metadata_proxy_group or os.getegid() - return user, group + def _get_metadata_proxy_user_group_watchlog(cls, conf): + user = conf.metadata_proxy_user or str(os.geteuid()) + group = conf.metadata_proxy_group or str(os.getegid()) + + watch_log = conf.metadata_proxy_watch_log + if watch_log is None: + # NOTE(cbrandily): Commonly, log watching can be enabled only + # when metadata proxy user is agent effective user (id/name). + watch_log = utils.is_effective_user(user) + + return user, group, watch_log @classmethod def _get_metadata_proxy_callback(cls, port, conf, network_id=None, @@ -131,7 +150,8 @@ class MetadataDriver(advanced_service.AdvancedService): def callback(pid_file): metadata_proxy_socket = conf.metadata_proxy_socket - user, group = cls._get_metadata_proxy_user_group(conf) + user, group, watch_log = ( + cls._get_metadata_proxy_user_group_watchlog(conf)) proxy_cmd = ['neutron-ns-metadata-proxy', '--pid_file=%s' % pid_file, '--metadata_proxy_socket=%s' % metadata_proxy_socket, @@ -141,7 +161,8 @@ class MetadataDriver(advanced_service.AdvancedService): '--metadata_proxy_user=%s' % user, '--metadata_proxy_group=%s' % group] proxy_cmd.extend(config.get_log_args( - conf, 'neutron-ns-metadata-proxy-%s.log' % uuid)) + conf, 'neutron-ns-metadata-proxy-%s.log' % uuid, + metadata_proxy_watch_log=watch_log)) return proxy_cmd return callback diff --git a/neutron/agent/metadata/namespace_proxy.py b/neutron/agent/metadata/namespace_proxy.py index 032e489fb..e84a256de 100644 --- a/neutron/agent/metadata/namespace_proxy.py +++ b/neutron/agent/metadata/namespace_proxy.py @@ -110,10 +110,10 @@ class NetworkMetadataProxyHandler(object): class ProxyDaemon(daemon.Daemon): def __init__(self, pidfile, port, network_id=None, router_id=None, - user=None, group=None): + user=None, group=None, watch_log=True): uuid = network_id or router_id super(ProxyDaemon, self).__init__(pidfile, uuid=uuid, user=user, - group=group) + group=group, watch_log=watch_log) self.network_id = network_id self.router_id = router_id self.port = port @@ -160,6 +160,11 @@ def main(): default=None, help=_("Group (gid or name) running metadata proxy after " "its initialization")), + cfg.BoolOpt('metadata_proxy_watch_log', + default=True, + help=_("Watch file log. Log watch should be disabled when " + "metadata_proxy_user/group has no read/write " + "permissions on metadata proxy log file.")), ] cfg.CONF.register_cli_opts(opts) @@ -173,7 +178,8 @@ def main(): network_id=cfg.CONF.network_id, router_id=cfg.CONF.router_id, user=cfg.CONF.metadata_proxy_user, - group=cfg.CONF.metadata_proxy_group) + group=cfg.CONF.metadata_proxy_group, + watch_log=cfg.CONF.metadata_proxy_watch_log) if cfg.CONF.daemonize: proxy.start() diff --git a/neutron/tests/unit/agent/linux/test_utils.py b/neutron/tests/unit/agent/linux/test_utils.py index c66d48ec6..ff0678ee9 100644 --- a/neutron/tests/unit/agent/linux/test_utils.py +++ b/neutron/tests/unit/agent/linux/test_utils.py @@ -209,7 +209,18 @@ class TestPathUtilities(base.BaseTestCase): ['/usr/bin/ping6', '8.8.8.8'])) +class FakeUser(object): + def __init__(self, name): + self.pw_name = name + + class TestBaseOSUtils(base.BaseTestCase): + + EUID = 123 + EUNAME = 'user' + EGID = 456 + EGNAME = 'group' + @mock.patch.object(os.path, 'isdir', return_value=False) @mock.patch.object(os, 'makedirs') def test_ensure_dir_not_exist(self, makedirs, isdir): @@ -224,6 +235,34 @@ class TestBaseOSUtils(base.BaseTestCase): isdir.assert_called_once_with('/the') self.assertFalse(makedirs.called) + @mock.patch('os.geteuid', return_value=EUID) + @mock.patch('pwd.getpwuid', return_value=FakeUser(EUNAME)) + def test_is_effective_user_id(self, getpwuid, geteuid): + self.assertTrue(utils.is_effective_user(self.EUID)) + geteuid.assert_called_once_with() + self.assertFalse(getpwuid.called) + + @mock.patch('os.geteuid', return_value=EUID) + @mock.patch('pwd.getpwuid', return_value=FakeUser(EUNAME)) + def test_is_effective_user_str_id(self, getpwuid, geteuid): + self.assertTrue(utils.is_effective_user(str(self.EUID))) + geteuid.assert_called_once_with() + self.assertFalse(getpwuid.called) + + @mock.patch('os.geteuid', return_value=EUID) + @mock.patch('pwd.getpwuid', return_value=FakeUser(EUNAME)) + def test_is_effective_user_name(self, getpwuid, geteuid): + self.assertTrue(utils.is_effective_user(self.EUNAME)) + geteuid.assert_called_once_with() + getpwuid.assert_called_once_with(self.EUID) + + @mock.patch('os.geteuid', return_value=EUID) + @mock.patch('pwd.getpwuid', return_value=FakeUser(EUNAME)) + def test_is_not_effective_user(self, getpwuid, geteuid): + self.assertFalse(utils.is_effective_user('wrong')) + geteuid.assert_called_once_with() + getpwuid.assert_called_once_with(self.EUID) + class TestUnixDomainHttpConnection(base.BaseTestCase): def test_connect(self): diff --git a/neutron/tests/unit/agent/metadata/test_driver.py b/neutron/tests/unit/agent/metadata/test_driver.py index 025b78a41..10cbc6d67 100644 --- a/neutron/tests/unit/agent/metadata/test_driver.py +++ b/neutron/tests/unit/agent/metadata/test_driver.py @@ -61,6 +61,7 @@ class TestMetadataDriverProcess(base.BaseTestCase): EUID = 123 EGID = 456 + EUNAME = 'neutron' def setUp(self): super(TestMetadataDriverProcess, self).setUp() @@ -79,11 +80,13 @@ class TestMetadataDriverProcess(base.BaseTestCase): cfg.CONF.register_opts(metadata_driver.MetadataDriver.OPTS) def _test_spawn_metadata_proxy(self, expected_user, expected_group, - user='', group=''): + user='', group='', watch_log=True): router_id = _uuid() router_ns = 'qrouter-%s' % router_id metadata_port = 8080 ip_class_path = 'neutron.agent.linux.ip_lib.IPWrapper' + is_effective_user = 'neutron.agent.linux.utils.is_effective_user' + fake_is_effective_user = lambda x: x in [self.EUNAME, str(self.EUID)] cfg.CONF.set_override('metadata_proxy_user', user) cfg.CONF.set_override('metadata_proxy_group', group) @@ -94,42 +97,58 @@ class TestMetadataDriverProcess(base.BaseTestCase): 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): + mock.patch(is_effective_user, + side_effect=fake_is_effective_user), + mock.patch(ip_class_path)) as ( + geteuid, getegid, is_effective_user, ip_mock): agent.metadata_driver.spawn_monitored_metadata_proxy( agent.process_monitor, router_ns, metadata_port, agent.conf, router_id=router_id) + netns_execute_args = [ + 'neutron-ns-metadata-proxy', + mock.ANY, + mock.ANY, + '--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', + '--verbose', + '--log-file=neutron-ns-metadata-proxy-%s.log' % + router_id] + if not watch_log: + netns_execute_args.append( + '--metadata_proxy_watch_log=false') ip_mock.assert_has_calls([ mock.call(namespace=router_ns), - mock.call().netns.execute([ - 'neutron-ns-metadata-proxy', - mock.ANY, - mock.ANY, - '--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', - '--verbose', - '--log-file=neutron-ns-metadata-proxy-%s.log' % - router_id - ], addl_env=None) + mock.call().netns.execute(netns_execute_args, 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_agent_user(self): + self._test_spawn_metadata_proxy( + self.EUNAME, str(self.EGID), user=self.EUNAME) - def test_spawn_metadata_proxy_with_uid(self): - self._test_spawn_metadata_proxy('321', self.EGID, user='321') + def test_spawn_metadata_proxy_with_nonagent_user(self): + self._test_spawn_metadata_proxy( + 'notneutron', str(self.EGID), user='notneutron', watch_log=False) + + def test_spawn_metadata_proxy_with_agent_uid(self): + self._test_spawn_metadata_proxy( + str(self.EUID), str(self.EGID), user=str(self.EUID)) + + def test_spawn_metadata_proxy_with_nonagent_uid(self): + self._test_spawn_metadata_proxy( + '321', str(self.EGID), user='321', watch_log=False) def test_spawn_metadata_proxy_with_group(self): - self._test_spawn_metadata_proxy(self.EUID, 'group', group='group') + self._test_spawn_metadata_proxy(str(self.EUID), 'group', group='group') def test_spawn_metadata_proxy_with_gid(self): - self._test_spawn_metadata_proxy(self.EUID, '654', group='654') + self._test_spawn_metadata_proxy(str(self.EUID), '654', group='654') def test_spawn_metadata_proxy(self): - self._test_spawn_metadata_proxy(self.EUID, self.EGID) + self._test_spawn_metadata_proxy(str(self.EUID), str(self.EGID)) diff --git a/neutron/tests/unit/test_metadata_namespace_proxy.py b/neutron/tests/unit/test_metadata_namespace_proxy.py index ce9c2bca9..8cf8d1415 100644 --- a/neutron/tests/unit/test_metadata_namespace_proxy.py +++ b/neutron/tests/unit/test_metadata_namespace_proxy.py @@ -292,7 +292,8 @@ class TestProxyDaemon(base.BaseTestCase): router_id='router_id', network_id=None, user=mock.ANY, - group=mock.ANY), + group=mock.ANY, + watch_log=mock.ANY), mock.call().start()] ) @@ -315,6 +316,7 @@ class TestProxyDaemon(base.BaseTestCase): router_id='router_id', network_id=None, user=mock.ANY, - group=mock.ANY), + group=mock.ANY, + watch_log=mock.ANY), mock.call().run()] )