diff --git a/etc/nova/rootwrap.d/compute.filters b/etc/nova/rootwrap.d/compute.filters index 33e0360b2560..8796050455b0 100644 --- a/etc/nova/rootwrap.d/compute.filters +++ b/etc/nova/rootwrap.d/compute.filters @@ -37,24 +37,16 @@ blkid: CommandFilter, blkid, root # nova/virt/disk/mount/nbd.py: 'blockdev', '--flushbufs', device blockdev: RegExpFilter, blockdev, root, blockdev, (--getsize64|--flushbufs), /dev/.* -# nova/virt/disk/vfs/localfs.py: 'tee', canonpath # nova/virt/libvirt/guest.py: 'tee', # nova/virt/libvirt/vif.py: utils.execute('tee', tee: CommandFilter, tee, root -# nova/virt/disk/vfs/localfs.py: 'mkdir', canonpath -mkdir: CommandFilter, mkdir, root - -# nova/virt/disk/vfs/localfs.py: 'chown' # nova/virt/libvirt/utils.py: def chown(): execute('chown', owner, path, # nova/virt/libvirt/driver.py: 'chown', os.getuid( console_log # nova/virt/libvirt/driver.py: 'chown', os.getuid( console_log # nova/virt/libvirt/driver.py: 'chown', 'root', basepath('disk') chown: CommandFilter, chown, root -# nova/virt/disk/vfs/localfs.py: 'chmod' -chmod: CommandFilter, chmod, root - # nova/virt/libvirt/vif.py: 'ip', 'tuntap', 'add', dev, 'mode', 'tap' # nova/virt/libvirt/vif.py: 'ip', 'link', 'set', dev, 'up' # nova/virt/libvirt/vif.py: 'ip', 'link', 'delete', dev @@ -180,9 +172,6 @@ mkfs: CommandFilter, mkfs, root # nova/virt/libvirt/utils.py: 'qemu-img' qemu-img: CommandFilter, qemu-img, root -# nova/virt/disk/vfs/localfs.py: 'readlink', '-e' -readlink: CommandFilter, readlink, root - # nova/virt/disk/api.py: mkfs.ext3: CommandFilter, mkfs.ext3, root mkfs.ext4: CommandFilter, mkfs.ext4, root @@ -200,11 +189,6 @@ lvs: CommandFilter, lvs, root # nova/virt/libvirt/utils.py: vgs: CommandFilter, vgs, root -# nova/utils.py: read_file_as_root: 'cat', file_path -# (called from nova/virt/disk/vfs/localfs.py:VFSLocalFS.read_file) -read_passwd: RegExpFilter, cat, root, cat, (/var|/usr)?/tmp/openstack-vfs-localfs[^/]+/etc/passwd -read_shadow: RegExpFilter, cat, root, cat, (/var|/usr)?/tmp/openstack-vfs-localfs[^/]+/etc/shadow - # os-brick needed commands read_initiator: ReadFileFilter, /etc/iscsi/initiatorname.iscsi multipath: CommandFilter, multipath, root @@ -222,7 +206,10 @@ scsi_id: CommandFilter, /lib/udev/scsi_id, root # os_brick.privileged.default oslo.privsep context # This line ties the superuser privs with the config files, context name, # and (implicitly) the actual python code invoked. -privsep-rootwrap: RegExpFilter, privsep-helper, root, privsep-helper, --config-file, /etc/(?!\.\.).*, --privsep_context, os_brick.privileged.default, --privsep_sock_path, /tmp/.* +privsep-rootwrap-os_brick: RegExpFilter, privsep-helper, root, privsep-helper, --config-file, /etc/(?!\.\.).*, --privsep_context, os_brick.privileged.default, --privsep_sock_path, /tmp/.* + +privsep-rootwrap-dac_admin: RegExpFilter, privsep-helper, root, privsep-helper, --config-file, /etc/(?!\.\.).*, --privsep_context, nova.privsep.dac_admin_pctxt, --privsep_sock_path, /tmp/.* + # nova/virt/libvirt/storage/dmcrypt.py: cryptsetup: CommandFilter, cryptsetup, root diff --git a/nova/privsep/__init__.py b/nova/privsep/__init__.py new file mode 100644 index 000000000000..608a040790b3 --- /dev/null +++ b/nova/privsep/__init__.py @@ -0,0 +1,31 @@ +# Copyright 2016 Red Hat, Inc +# Copyright 2017 Rackspace Australia +# +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +"""Setup privsep decorator.""" + +from oslo_privsep import priv_context + +# NOTE(tonyb): DAC == Discriminatory Access Control. Basically this context +# can bypass permissions checks in the file-system. +dac_admin_pctxt = priv_context.PrivContext( + 'nova', + cfg_section='nova_privileged', + pypath=__name__ + '.dac_admin_pctxt', + # NOTE(tonyb): These map to CAP_CHOWN, CAP_DAC_OVERRIDE, + # CAP_DAC_READ_SEARCH and CAP_FOWNER. Some do not have + # symbolic names in oslo.privsep yet. See capabilites(7) + # for more information + capabilities=[0, 1, 2, 3], +) diff --git a/nova/privsep/dac_admin.py b/nova/privsep/dac_admin.py new file mode 100644 index 000000000000..d43c26dfe911 --- /dev/null +++ b/nova/privsep/dac_admin.py @@ -0,0 +1,72 @@ +# Copyright 2016 Red Hat, Inc +# Copyright 2017 Rackspace Australia +# +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +"""Routines that use the dac_admin_pctxt to bypass file-system checks""" + +import os + +from oslo_utils import fileutils + +from nova import exception +import nova.privsep + + +@nova.privsep.dac_admin_pctxt.entrypoint +def readfile(path): + if not os.path.exists(path): + raise exception.FileNotFound(file_path=path) + with open(path, 'r') as f: + return f.read() + + +@nova.privsep.dac_admin_pctxt.entrypoint +def writefile(path, mode, content): + if not os.path.exists(path): + raise exception.FileNotFound(file_path=path) + with open(path, mode) as f: + f.write(content) + + +@nova.privsep.dac_admin_pctxt.entrypoint +def readlink(path): + if not os.path.exists(path): + raise exception.FileNotFound(file_path=path) + return os.readlink(path) + + +@nova.privsep.dac_admin_pctxt.entrypoint +def chown(path, uid=-1, gid=-1): + if not os.path.exists(path): + raise exception.FileNotFound(file_path=path) + return os.chown(path, uid, gid) + + +@nova.privsep.dac_admin_pctxt.entrypoint +def makedirs(path): + fileutils.ensure_tree(path) + + +@nova.privsep.dac_admin_pctxt.entrypoint +def chmod(path, mode): + if not os.path.exists(path): + raise exception.FileNotFound(file_path=path) + os.chmod(path, mode) + + +class path(object): + @staticmethod + @nova.privsep.dac_admin_pctxt.entrypoint + def exists(path): + return os.path.exists(path) diff --git a/nova/test.py b/nova/test.py index 9502e68928f7..cbc27ea2fb91 100644 --- a/nova/test.py +++ b/nova/test.py @@ -313,6 +313,9 @@ class TestCase(testtools.TestCase): self.useFixture(nova_fixtures.ForbidNewLegacyNotificationFixture()) + # NOTE(mikal): make sure we don't load a privsep helper accidentally + self.useFixture(nova_fixtures.PrivsepNoHelperFixture()) + def _setup_cells(self): """Setup a normal cellsv2 environment. diff --git a/nova/tests/fixtures.py b/nova/tests/fixtures.py index 257ca8c19683..c3073bda3938 100644 --- a/nova/tests/fixtures.py +++ b/nova/tests/fixtures.py @@ -30,6 +30,7 @@ from oslo_concurrency import lockutils from oslo_config import cfg import oslo_messaging as messaging from oslo_messaging import conffixture as messaging_conffixture +from oslo_privsep import daemon as privsep_daemon from requests import adapters from wsgi_intercept import interceptor @@ -1538,3 +1539,29 @@ class PlacementFixture(fixtures.Fixture): endpoint_override=self.endpoint, headers={'x-auth-token': self.token}, raise_exc=False) + + +class UnHelperfulClientChannel(privsep_daemon._ClientChannel): + def __init__(self, context): + raise Exception('You have attempted to start a privsep helper. ' + 'This is not allowed in the gate, and ' + 'indicates a failure to have mocked your tests.') + + +class PrivsepNoHelperFixture(fixtures.Fixture): + """A fixture to catch failures to mock privsep's rootwrap helper. + + If you fail to mock away a privsep'd method in a unit test, then + you may well end up accidentally running the privsep rootwrap + helper. This will fail in the gate, but it fails in a way which + doesn't identify which test is missing a mock. Instead, we + raise an exception so that you at least know where you've missed + something. + """ + + def setUp(self): + super(PrivsepNoHelperFixture, self).setUp() + + self.useFixture(fixtures.MonkeyPatch( + 'oslo_privsep.daemon.RootwrapClientChannel', + UnHelperfulClientChannel)) diff --git a/nova/tests/unit/test_utils.py b/nova/tests/unit/test_utils.py index 144c2da6faef..33778cfa8a97 100644 --- a/nova/tests/unit/test_utils.py +++ b/nova/tests/unit/test_utils.py @@ -145,18 +145,6 @@ class GenericUtilsTestCase(test.NoDBTestCase): self.assertTrue([c for c in password if c in 'ABCDEFGHIJKLMNOPQRSTUVWXYZ']) - def test_read_file_as_root(self): - def fake_execute(*args, **kwargs): - if args[1] == 'bad': - raise processutils.ProcessExecutionError() - return 'fakecontents', None - - self.stub_out('nova.utils.execute', fake_execute) - contents = utils.read_file_as_root('good') - self.assertEqual(contents, 'fakecontents') - self.assertRaises(exception.FileNotFound, - utils.read_file_as_root, 'bad') - def test_temporary_chown(self): def fake_execute(*args, **kwargs): if args[0] == 'chown': diff --git a/nova/tests/unit/utils.py b/nova/tests/unit/utils.py index 1d5da29c5cbf..ca434bf68ad9 100644 --- a/nova/tests/unit/utils.py +++ b/nova/tests/unit/utils.py @@ -188,11 +188,6 @@ def is_linux(): return platform.system() == 'Linux' -def coreutils_readlink_available(): - _out, err = nova.utils.trycmd('readlink', '-nm', '/') - return err == '' - - test_dns_managers = [] diff --git a/nova/tests/unit/virt/disk/vfs/test_localfs.py b/nova/tests/unit/virt/disk/vfs/test_localfs.py index fbb81b49250b..bf00c507c42b 100644 --- a/nova/tests/unit/virt/disk/vfs/test_localfs.py +++ b/nova/tests/unit/virt/disk/vfs/test_localfs.py @@ -12,143 +12,48 @@ # License for the specific language governing permissions and limitations # under the License. +import grp +import pwd import tempfile +from collections import namedtuple import mock -from oslo_concurrency import processutils from nova import exception from nova import test -from nova.tests.unit import utils as tests_utils import nova.utils from nova.virt.disk.mount import nbd from nova.virt.disk.vfs import localfs as vfsimpl from nova.virt.image import model as imgmodel -dirs = [] -files = {} -commands = [] - - -def fake_execute(*args, **kwargs): - commands.append({"args": args, "kwargs": kwargs}) - - if args[0] == "readlink": - if args[1] == "-nm": - if args[2] in ["/scratch/dir/some/file", - "/scratch/dir/some/dir", - "/scratch/dir/other/dir", - "/scratch/dir/other/file"]: - return args[2], "" - elif args[1] == "-e": - if args[2] in files: - return args[2], "" - - return "", "No such file" - elif args[0] == "mkdir": - dirs.append(args[2]) - elif args[0] == "chown": - owner = args[1] - path = args[2] - if path not in files: - raise Exception("No such file: " + path) - - sep = owner.find(':') - if sep != -1: - user = owner[0:sep] - group = owner[sep + 1:] - else: - user = owner - group = None - - if user: - if user == "fred": - uid = 105 - else: - uid = 110 - files[path]["uid"] = uid - if group: - if group == "users": - gid = 500 - else: - gid = 600 - files[path]["gid"] = gid - elif args[0] == "chgrp": - group = args[1] - path = args[2] - if path not in files: - raise Exception("No such file: " + path) - - if group == "users": - gid = 500 - else: - gid = 600 - files[path]["gid"] = gid - elif args[0] == "chmod": - mode = args[1] - path = args[2] - if path not in files: - raise Exception("No such file: " + path) - - files[path]["mode"] = int(mode, 8) - elif args[0] == "cat": - path = args[1] - if path not in files: - files[path] = { - "content": "Hello World", - "gid": 100, - "uid": 100, - "mode": 0o700 - } - return files[path]["content"], "" - elif args[0] == "tee": - if args[1] == "-a": - path = args[2] - append = True - else: - path = args[1] - append = False - if path not in files: - files[path] = { - "content": "Hello World", - "gid": 100, - "uid": 100, - "mode": 0o700, - } - if append: - files[path]["content"] += kwargs["process_input"] - else: - files[path]["content"] = kwargs["process_input"] - - class VirtDiskVFSLocalFSTestPaths(test.NoDBTestCase): def setUp(self): super(VirtDiskVFSLocalFSTestPaths, self).setUp() - real_execute = processutils.execute - - def nonroot_execute(*cmd_parts, **kwargs): - kwargs.pop('run_as_root', None) - return real_execute(*cmd_parts, **kwargs) - - self.stub_out('oslo_concurrency.processutils.execute', nonroot_execute) - self.rawfile = imgmodel.LocalFileImage("/dummy.img", + self.rawfile = imgmodel.LocalFileImage('/dummy.img', imgmodel.FORMAT_RAW) - def test_check_safe_path(self): - if not tests_utils.coreutils_readlink_available(): - self.skipTest("coreutils readlink(1) unavailable") + # NOTE(mikal): mocking a decorator is non-trivial, so this is the + # best we can do. + + @mock.patch.object(nova.privsep.dac_admin, 'readlink') + def test_check_safe_path(self, read_link): vfs = vfsimpl.VFSLocalFS(self.rawfile) - vfs.imgdir = "/foo" + vfs.imgdir = '/foo' + + read_link.return_value = '/foo/etc/something.conf' + ret = vfs._canonical_path('etc/something.conf') self.assertEqual(ret, '/foo/etc/something.conf') - def test_check_unsafe_path(self): - if not tests_utils.coreutils_readlink_available(): - self.skipTest("coreutils readlink(1) unavailable") + @mock.patch.object(nova.privsep.dac_admin, 'readlink') + def test_check_unsafe_path(self, read_link): vfs = vfsimpl.VFSLocalFS(self.rawfile) - vfs.imgdir = "/foo" + vfs.imgdir = '/foo' + + read_link.return_value = '/etc/something.conf' + self.assertRaises(exception.Invalid, vfs._canonical_path, 'etc/../../../something.conf') @@ -158,244 +63,109 @@ class VirtDiskVFSLocalFSTest(test.NoDBTestCase): def setUp(self): super(VirtDiskVFSLocalFSTest, self).setUp() - self.qcowfile = imgmodel.LocalFileImage("/dummy.qcow2", + self.qcowfile = imgmodel.LocalFileImage('/dummy.qcow2', imgmodel.FORMAT_QCOW2) - self.rawfile = imgmodel.LocalFileImage("/dummy.img", + self.rawfile = imgmodel.LocalFileImage('/dummy.img', imgmodel.FORMAT_RAW) - def test_makepath(self): - global dirs, commands - dirs = [] - commands = [] - self.stub_out('oslo_concurrency.processutils.execute', fake_execute) - + @mock.patch.object(nova.privsep.dac_admin, 'readlink') + @mock.patch.object(nova.privsep.dac_admin, 'makedirs') + def test_makepath(self, mkdir, read_link): vfs = vfsimpl.VFSLocalFS(self.qcowfile) - vfs.imgdir = "/scratch/dir" - vfs.make_path("/some/dir") - vfs.make_path("/other/dir") + vfs.imgdir = '/scratch/dir' - self.assertEqual(dirs, - ["/scratch/dir/some/dir", "/scratch/dir/other/dir"]), + vfs.make_path('/some/dir') + read_link.assert_called() + mkdir.assert_called_with(read_link.return_value) - root_helper = nova.utils.get_root_helper() - self.assertEqual(commands, - [{'args': ('readlink', '-nm', - '/scratch/dir/some/dir'), - 'kwargs': {'run_as_root': True, - 'root_helper': root_helper}}, - {'args': ('mkdir', '-p', - '/scratch/dir/some/dir'), - 'kwargs': {'run_as_root': True, - 'root_helper': root_helper}}, - {'args': ('readlink', '-nm', - '/scratch/dir/other/dir'), - 'kwargs': {'run_as_root': True, - 'root_helper': root_helper}}, - {'args': ('mkdir', '-p', - '/scratch/dir/other/dir'), - 'kwargs': {'run_as_root': True, - 'root_helper': root_helper}}]) - - def test_append_file(self): - global files, commands - files = {} - commands = [] - self.stub_out('oslo_concurrency.processutils.execute', fake_execute) + read_link.reset_mock() + mkdir.reset_mock() + vfs.make_path('/other/dir') + read_link.assert_called() + mkdir.assert_called_with(read_link.return_value) + @mock.patch.object(nova.privsep.dac_admin, 'readlink') + @mock.patch.object(nova.privsep.dac_admin, 'writefile') + def test_append_file(self, write_file, read_link): vfs = vfsimpl.VFSLocalFS(self.qcowfile) - vfs.imgdir = "/scratch/dir" - vfs.append_file("/some/file", " Goodbye") + vfs.imgdir = '/scratch/dir' - self.assertIn("/scratch/dir/some/file", files) - self.assertEqual(files["/scratch/dir/some/file"]["content"], - "Hello World Goodbye") + vfs.append_file('/some/file', ' Goodbye') - root_helper = nova.utils.get_root_helper() - self.assertEqual(commands, - [{'args': ('readlink', '-nm', - '/scratch/dir/some/file'), - 'kwargs': {'run_as_root': True, - 'root_helper': root_helper}}, - {'args': ('tee', '-a', - '/scratch/dir/some/file'), - 'kwargs': {'process_input': ' Goodbye', - 'run_as_root': True, - 'root_helper': root_helper}}]) - - def test_replace_file(self): - global files, commands - files = {} - commands = [] - self.stub_out('oslo_concurrency.processutils.execute', fake_execute) + read_link.assert_called() + write_file.assert_called_with(read_link.return_value, 'a', ' Goodbye') + @mock.patch.object(nova.privsep.dac_admin, 'readlink') + @mock.patch.object(nova.privsep.dac_admin, 'writefile') + def test_replace_file(self, write_file, read_link): vfs = vfsimpl.VFSLocalFS(self.qcowfile) - vfs.imgdir = "/scratch/dir" - vfs.replace_file("/some/file", "Goodbye") + vfs.imgdir = '/scratch/dir' - self.assertIn("/scratch/dir/some/file", files) - self.assertEqual(files["/scratch/dir/some/file"]["content"], - "Goodbye") + vfs.replace_file('/some/file', 'Goodbye') - root_helper = nova.utils.get_root_helper() - self.assertEqual(commands, - [{'args': ('readlink', '-nm', - '/scratch/dir/some/file'), - 'kwargs': {'run_as_root': True, - 'root_helper': root_helper}}, - {'args': ('tee', '/scratch/dir/some/file'), - 'kwargs': {'process_input': 'Goodbye', - 'run_as_root': True, - 'root_helper': root_helper}}]) - - def test_read_file(self): - global commands, files - files = {} - commands = [] - self.stub_out('oslo_concurrency.processutils.execute', fake_execute) + read_link.assert_called() + write_file.assert_called_with(read_link.return_value, 'w', 'Goodbye') + @mock.patch.object(nova.privsep.dac_admin, 'readlink') + @mock.patch.object(nova.privsep.dac_admin, 'readfile') + def test_read_file(self, read_file, read_link): vfs = vfsimpl.VFSLocalFS(self.qcowfile) - vfs.imgdir = "/scratch/dir" - self.assertEqual(vfs.read_file("/some/file"), "Hello World") + vfs.imgdir = '/scratch/dir' - root_helper = nova.utils.get_root_helper() - self.assertEqual(commands, - [{'args': ('readlink', '-nm', - '/scratch/dir/some/file'), - 'kwargs': {'run_as_root': True, - 'root_helper': root_helper}}, - {'args': ('cat', '/scratch/dir/some/file'), - 'kwargs': {'run_as_root': True, - 'root_helper': root_helper}}]) - - def test_has_file(self): - global commands, files - files = {} - commands = [] - self.stub_out('oslo_concurrency.processutils.execute', fake_execute) + self.assertEqual(read_file.return_value, vfs.read_file('/some/file')) + read_link.assert_called() + read_file.assert_called() + @mock.patch.object(nova.privsep.dac_admin.path, 'exists') + def test_has_file(self, exists): vfs = vfsimpl.VFSLocalFS(self.qcowfile) - vfs.imgdir = "/scratch/dir" - vfs.read_file("/some/file") - - self.assertTrue(vfs.has_file("/some/file")) - self.assertFalse(vfs.has_file("/other/file")) - - root_helper = nova.utils.get_root_helper() - self.assertEqual(commands, - [{'args': ('readlink', '-nm', - '/scratch/dir/some/file'), - 'kwargs': {'run_as_root': True, - 'root_helper': root_helper}}, - {'args': ('cat', '/scratch/dir/some/file'), - 'kwargs': {'run_as_root': True, - 'root_helper': root_helper}}, - {'args': ('readlink', '-nm', - '/scratch/dir/some/file'), - 'kwargs': {'run_as_root': True, - 'root_helper': root_helper}}, - {'args': ('readlink', '-e', - '/scratch/dir/some/file'), - 'kwargs': {'run_as_root': True, - 'root_helper': root_helper}}, - {'args': ('readlink', '-nm', - '/scratch/dir/other/file'), - 'kwargs': {'run_as_root': True, - 'root_helper': root_helper}}, - {'args': ('readlink', '-e', - '/scratch/dir/other/file'), - 'kwargs': {'run_as_root': True, - 'root_helper': root_helper}}, - ]) - - def test_set_permissions(self): - global commands, files - commands = [] - files = {} - self.stub_out('oslo_concurrency.processutils.execute', fake_execute) + vfs.imgdir = '/scratch/dir' + has = vfs.has_file('/some/file') + self.assertEqual(exists.return_value, has) + @mock.patch.object(nova.privsep.dac_admin, 'readlink') + @mock.patch.object(nova.privsep.dac_admin, 'chmod') + def test_set_permissions(self, chmod, read_link): vfs = vfsimpl.VFSLocalFS(self.qcowfile) - vfs.imgdir = "/scratch/dir" - vfs.read_file("/some/file") + vfs.imgdir = '/scratch/dir' - vfs.set_permissions("/some/file", 0o777) - self.assertEqual(files["/scratch/dir/some/file"]["mode"], 0o777) - - root_helper = nova.utils.get_root_helper() - self.assertEqual(commands, - [{'args': ('readlink', '-nm', - '/scratch/dir/some/file'), - 'kwargs': {'run_as_root': True, - 'root_helper': root_helper}}, - {'args': ('cat', '/scratch/dir/some/file'), - 'kwargs': {'run_as_root': True, - 'root_helper': root_helper}}, - {'args': ('readlink', '-nm', - '/scratch/dir/some/file'), - 'kwargs': {'run_as_root': True, - 'root_helper': root_helper}}, - {'args': ('chmod', '777', - '/scratch/dir/some/file'), - 'kwargs': {'run_as_root': True, - 'root_helper': root_helper}}]) - - def test_set_ownership(self): - global commands, files - commands = [] - files = {} - self.stub_out('oslo_concurrency.processutils.execute', fake_execute) + vfs.set_permissions('/some/file', 0o777) + read_link.assert_called() + chmod.assert_called_with(read_link.return_value, 0o777) + @mock.patch.object(nova.privsep.dac_admin, 'readlink') + @mock.patch.object(nova.privsep.dac_admin, 'chown') + @mock.patch.object(pwd, 'getpwnam') + @mock.patch.object(grp, 'getgrnam') + def test_set_ownership(self, getgrnam, getpwnam, chown, read_link): vfs = vfsimpl.VFSLocalFS(self.qcowfile) - vfs.imgdir = "/scratch/dir" - vfs.read_file("/some/file") + vfs.imgdir = '/scratch/dir' - self.assertEqual(files["/scratch/dir/some/file"]["uid"], 100) - self.assertEqual(files["/scratch/dir/some/file"]["gid"], 100) + fake_passwd = namedtuple('fake_passwd', 'pw_uid') + getpwnam.return_value(fake_passwd(pw_uid=100)) - vfs.set_ownership("/some/file", "fred", None) - self.assertEqual(files["/scratch/dir/some/file"]["uid"], 105) - self.assertEqual(files["/scratch/dir/some/file"]["gid"], 100) + fake_group = namedtuple('fake_group', 'gr_gid') + getgrnam.return_value(fake_group(gr_gid=101)) - vfs.set_ownership("/some/file", None, "users") - self.assertEqual(files["/scratch/dir/some/file"]["uid"], 105) - self.assertEqual(files["/scratch/dir/some/file"]["gid"], 500) + vfs.set_ownership('/some/file', 'fred', None) + read_link.assert_called() + chown.assert_called_with(read_link.return_value, + uid=getpwnam.return_value.pw_uid) - vfs.set_ownership("/some/file", "joe", "admins") - self.assertEqual(files["/scratch/dir/some/file"]["uid"], 110) - self.assertEqual(files["/scratch/dir/some/file"]["gid"], 600) + read_link.reset_mock() + chown.reset_mock() + vfs.set_ownership('/some/file', None, 'users') + read_link.assert_called() + chown.assert_called_with(read_link.return_value, + gid=getgrnam.return_value.gr_gid) - root_helper = nova.utils.get_root_helper() - self.assertEqual(commands, - [{'args': ('readlink', '-nm', - '/scratch/dir/some/file'), - 'kwargs': {'run_as_root': True, - 'root_helper': root_helper}}, - {'args': ('cat', '/scratch/dir/some/file'), - 'kwargs': {'run_as_root': True, - 'root_helper': root_helper}}, - {'args': ('readlink', '-nm', - '/scratch/dir/some/file'), - 'kwargs': {'run_as_root': True, - 'root_helper': root_helper}}, - {'args': ('chown', 'fred', - '/scratch/dir/some/file'), - 'kwargs': {'run_as_root': True, - 'root_helper': root_helper}}, - {'args': ('readlink', '-nm', - '/scratch/dir/some/file'), - 'kwargs': {'run_as_root': True, - 'root_helper': root_helper}}, - {'args': ('chgrp', 'users', - '/scratch/dir/some/file'), - 'kwargs': {'run_as_root': True, - 'root_helper': root_helper}}, - {'args': ('readlink', '-nm', - '/scratch/dir/some/file'), - 'kwargs': {'run_as_root': True, - 'root_helper': root_helper}}, - {'args': ('chown', 'joe:admins', - '/scratch/dir/some/file'), - 'kwargs': {'run_as_root': True, - 'root_helper': root_helper}}]) + read_link.reset_mock() + chown.reset_mock() + vfs.set_ownership('/some/file', 'joe', 'admins') + read_link.assert_called() + chown.assert_called_with(read_link.return_value, + uid=getpwnam.return_value.pw_uid, + gid=getgrnam.return_value.gr_gid) @mock.patch.object(nova.utils, 'execute') def test_get_format_fs(self, execute): @@ -441,7 +211,7 @@ class VirtDiskVFSLocalFSTest(test.NoDBTestCase): vfs.setup() self.assertTrue(mkdtemp.called) - NbdMount.assert_called_once_with(self.qcowfile, "tmp/", None) + NbdMount.assert_called_once_with(self.qcowfile, 'tmp/', None) mounter.do_mount.assert_called_once_with() @mock.patch.object(tempfile, 'mkdtemp') @@ -456,5 +226,5 @@ class VirtDiskVFSLocalFSTest(test.NoDBTestCase): vfs.setup(mount=False) self.assertTrue(mkdtemp.called) - NbdMount.assert_called_once_with(self.qcowfile, "tmp/", None) + NbdMount.assert_called_once_with(self.qcowfile, 'tmp/', None) self.assertFalse(mounter.do_mount.called) diff --git a/nova/tests/unit/virt/xenapi/test_xenapi.py b/nova/tests/unit/virt/xenapi/test_xenapi.py index e0cf2dea0797..dbdbdcefd891 100644 --- a/nova/tests/unit/virt/xenapi/test_xenapi.py +++ b/nova/tests/unit/virt/xenapi/test_xenapi.py @@ -949,55 +949,25 @@ class XenAPIVMTestCase(stubs.XenAPITestBase, @testtools.skipIf(test_utils.is_osx(), 'IPv6 pretty-printing broken on OSX, see bug 1409135') - def test_spawn_netinject_file(self): + @mock.patch.object(nova.privsep.dac_admin, 'readlink') + @mock.patch.object(nova.privsep.dac_admin, 'writefile') + @mock.patch.object(nova.privsep.dac_admin, 'makedirs') + @mock.patch.object(nova.privsep.dac_admin, 'chown') + @mock.patch.object(nova.privsep.dac_admin, 'chmod') + def test_spawn_netinject_file(self, chmod, chown, mkdir, write_file, + read_link): self.flags(flat_injected=True) db_fakes.stub_out_db_instance_api(self, injected=True) - self._tee_executed = False - - def _tee_handler(cmd, **kwargs): - actual = kwargs.get('process_input', None) - expected = """\ -# Injected by Nova on instance boot -# -# This file describes the network interfaces available on your system -# and how to activate them. For more information, see interfaces(5). - -# The loopback network interface -auto lo -iface lo inet loopback - -auto eth0 -iface eth0 inet static - hwaddress ether DE:AD:BE:EF:00:01 - address 192.168.1.100 - netmask 255.255.255.0 - broadcast 192.168.1.255 - gateway 192.168.1.1 - dns-nameservers 192.168.1.3 192.168.1.4 -iface eth0 inet6 static - hwaddress ether DE:AD:BE:EF:00:01 - address 2001:db8:0:1:dcad:beff:feef:1 - netmask 64 - gateway 2001:db8:0:1::1 -""" - self.assertEqual(expected, actual) - self._tee_executed = True - return '', '' - - def _readlink_handler(cmd_parts, **kwargs): - return os.path.realpath(cmd_parts[2]), '' - - fake_processutils.fake_execute_set_repliers([ - # Capture the tee .../etc/network/interfaces command - (r'tee.*interfaces', _tee_handler), - (r'readlink -nm.*', _readlink_handler), - ]) self._test_spawn(IMAGE_MACHINE, IMAGE_KERNEL, IMAGE_RAMDISK, check_injection=True) - self.assertTrue(self._tee_executed) + read_link.assert_called() + mkdir.assert_called() + chown.assert_called() + chmod.assert_called() + write_file.assert_called() @testtools.skipIf(test_utils.is_osx(), 'IPv6 pretty-printing broken on OSX, see bug 1409135') diff --git a/nova/utils.py b/nova/utils.py index 6ee945b03524..55f2fcb7cf75 100644 --- a/nova/utils.py +++ b/nova/utils.py @@ -666,15 +666,6 @@ def generate_mac_address(): return ':'.join(map(lambda x: "%02x" % x, mac)) -def read_file_as_root(file_path): - """Secure helper to read file as root.""" - try: - out, _err = execute('cat', file_path, run_as_root=True) - return out - except processutils.ProcessExecutionError: - raise exception.FileNotFound(file_path=file_path) - - @contextlib.contextmanager def temporary_chown(path, owner_uid=None): """Temporarily chown a path. diff --git a/nova/virt/disk/vfs/localfs.py b/nova/virt/disk/vfs/localfs.py index 4eaf489c1eba..933cbbe080fa 100644 --- a/nova/virt/disk/vfs/localfs.py +++ b/nova/virt/disk/vfs/localfs.py @@ -1,4 +1,5 @@ # Copyright 2012 Red Hat, Inc. +# Copyright 2017 Rackspace Australia # # Licensed under the Apache License, Version 2.0 (the "License"); you may # not use this file except in compliance with the License. You may obtain @@ -12,7 +13,9 @@ # License for the specific language governing permissions and limitations # under the License. +import grp import os +import pwd import tempfile from oslo_log import log as logging @@ -20,6 +23,7 @@ from oslo_utils import excutils from nova import exception from nova.i18n import _ +import nova.privsep.dac_admin from nova import utils from nova.virt.disk.mount import api as mount_api from nova.virt.disk.vfs import api as vfs @@ -37,10 +41,7 @@ class VFSLocalFS(vfs.VFS): path with '..' in it will hit this safeguard. """ def _canonical_path(self, path): - canonpath, _err = utils.execute( - 'readlink', '-nm', - os.path.join(self.imgdir, path.lstrip("/")), - run_as_root=True) + canonpath = nova.privsep.dac_admin.readlink(path) if not canonpath.startswith(os.path.realpath(self.imgdir) + '/'): raise exception.Invalid(_('File path %s not valid') % path) return canonpath @@ -99,64 +100,45 @@ class VFSLocalFS(vfs.VFS): def make_path(self, path): LOG.debug("Make directory path=%s", path) - canonpath = self._canonical_path(path) - utils.execute('mkdir', '-p', canonpath, run_as_root=True) + nova.privsep.dac_admin.makedirs(self._canonical_path(path)) def append_file(self, path, content): LOG.debug("Append file path=%s", path) - canonpath = self._canonical_path(path) - - args = ["-a", canonpath] - kwargs = dict(process_input=content, run_as_root=True) - - utils.execute('tee', *args, **kwargs) + return nova.privsep.dac_admin.writefile( + self._canonical_path(path), 'a', content) def replace_file(self, path, content): LOG.debug("Replace file path=%s", path) - canonpath = self._canonical_path(path) - - args = [canonpath] - kwargs = dict(process_input=content, run_as_root=True) - - utils.execute('tee', *args, **kwargs) + return nova.privsep.dac_admin.writefile( + self._canonical_path(path), 'w', content) def read_file(self, path): LOG.debug("Read file path=%s", path) - canonpath = self._canonical_path(path) - - return utils.read_file_as_root(canonpath) + return nova.privsep.dac_admin.readfile(self._canonical_path(path)) def has_file(self, path): + # NOTE(mikal): it is deliberate that we don't generate a canonical + # path here, as that tests for existance and would raise an exception. LOG.debug("Has file path=%s", path) - canonpath = self._canonical_path(path) - exists, _err = utils.trycmd('readlink', '-e', - canonpath, - run_as_root=True) - return exists + return nova.privsep.dac_admin.path.exists(path) def set_permissions(self, path, mode): LOG.debug("Set permissions path=%(path)s mode=%(mode)o", {'path': path, 'mode': mode}) - canonpath = self._canonical_path(path) - utils.execute('chmod', "%o" % mode, canonpath, run_as_root=True) + nova.privsep.dac_admin.chmod(self._canonical_path(path), mode) def set_ownership(self, path, user, group): LOG.debug("Set permissions path=%(path)s " "user=%(user)s group=%(group)s", {'path': path, 'user': user, 'group': group}) canonpath = self._canonical_path(path) - owner = None - cmd = "chown" - if group is not None and user is not None: - owner = user + ":" + group - elif user is not None: - owner = user - elif group is not None: - owner = group - cmd = "chgrp" - if owner is not None: - utils.execute(cmd, owner, canonpath, run_as_root=True) + chown_kwargs = {} + if user: + chown_kwargs['uid'] = pwd.getpwnam(user).pw_uid + if group: + chown_kwargs['gid'] = grp.getgrnam(group).gr_gid + nova.privsep.dac_admin.chown(canonpath, **chown_kwargs) def get_image_fs(self): if self.mount.device or self.mount.get_dev(): diff --git a/releasenotes/notes/privsep-queens-4548989d1cbe3aeb.yaml b/releasenotes/notes/privsep-queens-4548989d1cbe3aeb.yaml new file mode 100644 index 000000000000..f16638f8e497 --- /dev/null +++ b/releasenotes/notes/privsep-queens-4548989d1cbe3aeb.yaml @@ -0,0 +1,9 @@ +--- +security: + Privsep transitions. Nova is transitioning from using the older style + rootwrap privilege escalation path to the new style Oslo privsep path. + This should improve performance and security of Nova in the long term. + - | + privsep daemons are now started by nova when required. These daemons can + be started via rootwrap if required. rootwrap configs therefore need to + be updated to include new privsep daemon invocations. diff --git a/releasenotes/notes/privsep-queens-rootwrap-adds-907aa1bc8e3eb2ca.yaml b/releasenotes/notes/privsep-queens-rootwrap-adds-907aa1bc8e3eb2ca.yaml new file mode 100644 index 000000000000..b6d86736747c --- /dev/null +++ b/releasenotes/notes/privsep-queens-rootwrap-adds-907aa1bc8e3eb2ca.yaml @@ -0,0 +1,5 @@ +--- +upgrade: + - | + A dac-admin privsep daemon has been added and needs to be included in your + rootwrap configuration. \ No newline at end of file