Merge lp:~allenap/maas/atomic-delete-and-write-scripts into lp:~maas-committers/maas/trunk

[Gavin Panella]

New scripts maas-delete-file and maas-write-file which will be installed to /usr/lib/maas.

MAAS's access to writing and deleting files is <cough> not great. These scripts are an improvement in that they enforce access controls that are not possible to express using sudo.

These scripts are not yet actually used — that will come in a later branch — but landing them like this allows me to then get the packaging right before switching over.

[Blake Rouse (blake-rouse)]

Looks good. Getting the whitelist wrong will break MAAS, so I hope your sure that all files are in the whitelist.

review: Approve

[MAAS Lander (maas-lander)]

The attempt to merge lp:~allenap/maas/atomic-delete-and-write-scripts into lp:maas failed. Below is the output from the failed tests.

Hit:1 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial InRelease
Get:2 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial-updates InRelease [102 kB]
Get:3 http://prodstack-zone-2.clouds.archive.ubuntu.com/ubuntu xenial-backports InRelease [102 kB]
Get:4 http://security.ubuntu.com/ubuntu xenial-security InRelease [102 kB]
Fetched 306 kB in 0s (612 kB/s)
Reading package lists...
sudo DEBIAN_FRONTEND=noninteractive apt-get -y \
    --no-install-recommends install apache2 archdetect-deb authbind avahi-utils bash bind9 bind9utils build-essential bzr bzr-builddeb chromium-browser chromium-chromedriver curl daemontools debhelper dh-apport dh-systemd distro-info dnsutils firefox freeipmi-tools git gjs ipython isc-dhcp-common isc-dhcp-server libjs-angularjs libjs-jquery libjs-jquery-hotkeys libjs-yui3-full libjs-yui3-min libnss-wrapper libpq-dev make nodejs-legacy npm postgresql psmisc pxelinux python3-all python3-apt python3-attr python3-bson python3-convoy python3-crochet python3-cssselect python3-curtin python3-dev python3-distro-info python3-django python3-django-nose python3-django-piston3 python3-dnspython python3-docutils python3-formencode python3-hivex python3-httplib2 python3-jinja2 python3-jsonschema python3-lxml python3-netaddr python3-netifaces python3-novaclient python3-oauth python3-oauthlib python3-openssl python3-paramiko python3-petname python3-pexpect python3-psycopg2 python3-pyinotify python3-pyparsing python3-pyvmomi python3-requests python3-seamicroclient python3-setuptools python3-simplestreams python3-sphinx python3-tempita python3-twisted python3-txtftp python3-tz python3-yaml python3-zope.interface python-bson python-crochet python-django python-django-piston python-djorm-ext-pgarray python-formencode python-lxml python-netaddr python-netifaces python-pocket-lint python-psycopg2 python-simplejson python-tempita python-twisted python-yaml socat syslinux-common tgt ubuntu-cloudimage-keyring wget xvfb
Reading package lists...
Building dependency tree...
Reading state information...
authbind is already the newest version (2.1.1+nmu1).
avahi-utils is already the newest version (0.6.32~rc+dfsg-1ubuntu2).
build-essential is already the newest version (12.1ubuntu2).
debhelper is already the newest version (9.20160115ubuntu3).
distro-info is already the newest version (0.14build1).
git is already the newest version (1:2.7.4-0ubuntu1).
libjs-angularjs is already the newest version (1.2.28-1ubuntu2).
libjs-jquery is already the newest version (1.11.3+dfsg-4).
libjs-yui3-full is already the newest version (3.5.1-1ubuntu3).
libjs-yui3-min is already the newest version (3.5.1-1ubuntu3).
make is already the newest version (4.1-6).
postgresql is already the newest version (9.5+173).
psmisc is already the newest version (22.21-2.1build1).
pxelinux is already the newest version (3:6.03+dfsg-11ubuntu1).
python-formencode is already the newest version (1.3.0-0ubuntu5).
python-lxml is already the newest version (3.5.0-1build1).
python-netaddr is already the newest version (0.7.18-1).
py...

[Gavin Panella (allenap)]

Spurious test failure fixed in another branch.

=== added file 'scripts/maas-delete-file'
--- scripts/maas-delete-file	1970-01-01 00:00:00 +0000
+++ scripts/maas-delete-file	2017-03-22 16:56:50 +0000
@@ -0,0 +1,47 @@
+#!/usr/bin/env python3.5
+# Copyright 2017 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Atomically delete a file.
+
+The filename is checked against an internal white list. As such it's intended
+to be used behind `sudo`.
+"""
+
+import argparse
+import pipes
+
+from provisioningserver.utils.fs import atomic_delete
+
+
+whitelist = {
+    "/var/lib/maas/dhcpd.conf",
+    "/var/lib/maas/dhcpd6.conf",
+}
+
+
+arg_parser = argparse.ArgumentParser(description=__doc__)
+arg_parser.add_argument("filename", help="The file to delete.")
+
+
+def main(args):
+
+    # Validate the filename here because using a `choices` argument in the
+    # parser results in ugly help and error text.
+    if args.filename not in whitelist:
+        arg_parser.error(
+            "Given filename %s is not in the white list. "
+            "Choose from: %s." % (
+                pipes.quote(args.filename), ", ".join(
+                    map(pipes.quote, sorted(whitelist)))))
+
+    # Okay, good to go.
+    else:
+        try:
+            atomic_delete(args.filename)
+        except FileNotFoundError:
+            pass  # Ignore; it's already gone.
+
+
+if __name__ == "__main__":
+    main(arg_parser.parse_args())

=== added file 'scripts/maas-write-file'
--- scripts/maas-write-file	1970-01-01 00:00:00 +0000
+++ scripts/maas-write-file	2017-03-22 16:56:50 +0000
@@ -0,0 +1,64 @@
+#!/usr/bin/env python3.5
+# Copyright 2017 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Atomically write a file.
+
+Reads the byte content from standard-in and writes to the specified file. The
+filename is checked against an internal white list. As such it's intended to
+be used behind `sudo`.
+"""
+
+import argparse
+import pipes
+import sys
+
+from provisioningserver.utils.fs import atomic_write
+
+
+whitelist = {
+    "/etc/ntp.conf",
+    "/etc/ntp/maas.conf",
+    "/var/lib/maas/dhcpd-interfaces",
+    "/var/lib/maas/dhcpd.conf",
+    "/var/lib/maas/dhcpd6-interfaces",
+    "/var/lib/maas/dhcpd6.conf",
+}
+
+
+def octal(string):
+    """Parse `string` as an octal number."""
+    return int(string, 8)
+
+
+arg_parser = argparse.ArgumentParser(description=__doc__)
+arg_parser.add_argument("filename", help="The file to write.")
+arg_parser.add_argument("mode", type=octal, help="The octal file mode.")
+
+
+def main(args, fin):
+
+    # Validate the filename here because using a `choices` argument in the
+    # parser results in ugly help and error text.
+    if args.filename not in whitelist:
+        arg_parser.error(
+            "Given filename %s is not in the white list. "
+            "Choose from: %s." % (
+                pipes.quote(args.filename), ", ".join(
+                    map(pipes.quote, sorted(whitelist)))))
+
+    # Do not allow "high" bits in the mode, especially setuid and setgid.
+    elif args.mode & 0o777 != args.mode:
+        arg_parser.error(
+            "Given file mode 0o%o is not permitted; only "
+            "permission bits may be set." % args.mode)
+
+    # Okay, good to go.
+    else:
+        atomic_write(
+            fin.read(), args.filename, overwrite=True,
+            mode=args.mode)
+
+
+if __name__ == "__main__":
+    main(arg_parser.parse_args(), sys.stdin.buffer)

=== modified file 'setup.py'
--- setup.py	2016-12-07 12:46:14 +0000
+++ setup.py	2017-03-22 16:56:50 +0000
@@ -118,7 +118,9 @@
             ['scripts/maas-dhcp-helper']),
         ('/usr/lib/maas',
             ['scripts/maas-dhcp-monitor',
-             'scripts/maas-network-monitor']),
+             'scripts/maas-network-monitor',
+             'scripts/maas-delete-file',
+             'scripts/maas-write-file']),
     ],
 
     classifiers=[

=== modified file 'src/provisioningserver/utils/tests/test_fs.py'
--- src/provisioningserver/utils/tests/test_fs.py	2017-01-28 00:51:47 +0000
+++ src/provisioningserver/utils/tests/test_fs.py	2017-03-22 16:56:50 +0000
@@ -5,8 +5,10 @@
 
 __all__ = []
 
+import io
 import os
 import os.path
+import random
 from shutil import rmtree
 import stat
 from subprocess import (
@@ -15,16 +17,21 @@
 )
 import tempfile
 import time
+import tokenize
+import types
 from unittest.mock import (
     ANY,
     call,
+    create_autospec,
     Mock,
     sentinel,
 )
 
 from maastesting import root
 from maastesting.factory import factory
+from maastesting.fixtures import CaptureStandardIO
 from maastesting.matchers import (
+    DocTestMatches,
     FileContains,
     MockCalledOnceWith,
     MockCallsMatch,
@@ -53,12 +60,15 @@
 )
 import provisioningserver.utils.fs as fs_module
 from testtools.matchers import (
+    AllMatch,
     DirContains,
     DirExists,
     EndsWith,
     Equals,
     FileExists,
+    GreaterThan,
     HasLength,
+    IsInstance,
     MatchesRegex,
     Not,
     SamePath,
@@ -433,6 +443,122 @@
             sudo_delete_file, self.make_file())
 
 
+def load_script(filename):
+    """Load the Python script at `filename` into a new module."""
+    modname = os.path.relpath(filename, root).replace(os.sep, ".")
+    module = types.ModuleType(modname)
+    with tokenize.open(filename) as fd:
+        code = compile(fd.read(), filename, "exec", dont_inherit=True)
+        exec(code, module.__dict__, module.__dict__)
+    return module
+
+
+class TestSudoWriteFileScript(MAASTestCase):
+    """Tests for `scripts/maas-write-file`."""
+
+    def setUp(self):
+        super(TestSudoWriteFileScript, self).setUp()
+        self.script_path = os.path.join(root, "scripts/maas-write-file")
+        self.script = load_script(self.script_path)
+        self.script.atomic_write = create_autospec(self.script.atomic_write)
+
+    def test__white_list_is_a_non_empty_set_of_file_names(self):
+        self.assertThat(self.script.whitelist, IsInstance(set))
+        self.assertThat(self.script.whitelist, Not(HasLength(0)))
+        self.assertThat(self.script.whitelist, AllMatch(IsInstance(str)))
+
+    def test__accepts_file_names_on_white_list(self):
+        calls_expected = []
+        for filename in self.script.whitelist:
+            content = factory.make_bytes()  # It's binary safe.
+            mode = random.randint(0o000, 0o777)  # Inclusive of endpoints.
+            args = self.script.arg_parser.parse_args([filename, oct(mode)])
+            self.script.main(args, io.BytesIO(content))
+            calls_expected.append(call(
+                content, filename, overwrite=True, mode=mode))
+        self.assertThat(
+            self.script.atomic_write,
+            MockCallsMatch(*calls_expected))
+
+    def test__rejects_file_name_not_on_white_list(self):
+        filename = factory.make_name("/some/where", sep="/")
+        mode = random.randint(0o000, 0o777)  # Inclusive of endpoints.
+        args = self.script.arg_parser.parse_args([filename, oct(mode)])
+        with CaptureStandardIO() as stdio:
+            error = self.assertRaises(
+                SystemExit, self.script.main, args, io.BytesIO())
+        self.assertThat(error.code, GreaterThan(0))
+        self.assertThat(self.script.atomic_write, MockNotCalled())
+        self.assertThat(stdio.getOutput(), Equals(""))
+        self.assertThat(stdio.getError(), DocTestMatches(
+            "usage: ... Given filename ... is not in the "
+            "white list. Choose from: ..."
+        ))
+
+    def test__rejects_file_mode_with_high_bits_set(self):
+        filename = random.choice(list(self.script.whitelist))
+        mode = random.randint(0o1000, 0o7777)  # Inclusive of endpoints.
+        args = self.script.arg_parser.parse_args([filename, oct(mode)])
+        with CaptureStandardIO() as stdio:
+            error = self.assertRaises(
+                SystemExit, self.script.main, args, io.BytesIO())
+        self.assertThat(error.code, GreaterThan(0))
+        self.assertThat(self.script.atomic_write, MockNotCalled())
+        self.assertThat(stdio.getOutput(), Equals(""))
+        self.assertThat(stdio.getError(), DocTestMatches(
+            "usage: ... Given file mode 0o... is not permitted; "
+            "only permission bits may be set."
+        ))
+
+
+class TestSudoDeleteFileScript(MAASTestCase):
+    """Tests for `scripts/maas-delete-file`."""
+
+    def setUp(self):
+        super(TestSudoDeleteFileScript, self).setUp()
+        self.script_path = os.path.join(root, "scripts/maas-delete-file")
+        self.script = load_script(self.script_path)
+        self.script.atomic_delete = create_autospec(self.script.atomic_delete)
+
+    def test__white_list_is_a_non_empty_set_of_file_names(self):
+        self.assertThat(self.script.whitelist, IsInstance(set))
+        self.assertThat(self.script.whitelist, Not(HasLength(0)))
+        self.assertThat(self.script.whitelist, AllMatch(IsInstance(str)))
+
+    def test__accepts_file_names_on_white_list(self):
+        calls_expected = []
+        for filename in self.script.whitelist:
+            args = self.script.arg_parser.parse_args([filename])
+            self.script.main(args)
+            calls_expected.append(call(filename))
+        self.assertThat(
+            self.script.atomic_delete,
+            MockCallsMatch(*calls_expected))
+
+    def test__is_okay_when_the_file_does_not_exist(self):
+        filename = random.choice(list(self.script.whitelist))
+        args = self.script.arg_parser.parse_args([filename])
+        self.script.atomic_delete.side_effect = FileNotFoundError
+        self.script.main(args)
+        self.assertThat(
+            self.script.atomic_delete,
+            MockCalledOnceWith(filename))
+
+    def test__rejects_file_name_not_on_white_list(self):
+        filename = factory.make_name("/some/where", sep="/")
+        args = self.script.arg_parser.parse_args([filename])
+        with CaptureStandardIO() as stdio:
+            error = self.assertRaises(
+                SystemExit, self.script.main, args)
+        self.assertThat(error.code, GreaterThan(0))
+        self.assertThat(self.script.atomic_delete, MockNotCalled())
+        self.assertThat(stdio.getOutput(), Equals(""))
+        self.assertThat(stdio.getError(), DocTestMatches(
+            "usage: ... Given filename ... is not in the "
+            "white list. Choose from: ..."
+        ))
+
+
 class TestTempDir(MAASTestCase):
 
     def test_creates_real_fresh_directory(self):