From f535e8bb9905b5632416135af5789704db6d2867 Mon Sep 17 00:00:00 2001 From: Michael Still Date: Mon, 24 Apr 2017 14:51:42 +1000 Subject: [PATCH] First attempt at adding a privsep user to nova itself. I don't particularly care about this use case (although the localfs code should perhaps go away), but it was a nice contained example of a privsep user which wasn't just calling a command line. This patch also starts to layout what an API to the privsep'd code might look like. For now its modelled on python's os module, because that's where all the operations we perform are coming from. The rootwrap configuration is cleaned up as we remove users. Co-Authored-By: Tony Breeds Change-Id: I911cc51a226d6af29d63a7a2c69253de870073e9 --- etc/nova/rootwrap.d/compute.filters | 21 +- nova/privsep/__init__.py | 31 ++ nova/privsep/dac_admin.py | 72 +++ nova/test.py | 3 + nova/tests/fixtures.py | 27 ++ nova/tests/unit/test_utils.py | 12 - nova/tests/unit/utils.py | 5 - nova/tests/unit/virt/disk/vfs/test_localfs.py | 420 ++++-------------- nova/tests/unit/virt/xenapi/test_xenapi.py | 54 +-- nova/utils.py | 9 - nova/virt/disk/vfs/localfs.py | 60 +-- .../privsep-queens-4548989d1cbe3aeb.yaml | 9 + ...queens-rootwrap-adds-907aa1bc8e3eb2ca.yaml | 5 + 13 files changed, 279 insertions(+), 449 deletions(-) create mode 100644 nova/privsep/__init__.py create mode 100644 nova/privsep/dac_admin.py create mode 100644 releasenotes/notes/privsep-queens-4548989d1cbe3aeb.yaml create mode 100644 releasenotes/notes/privsep-queens-rootwrap-adds-907aa1bc8e3eb2ca.yaml 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