Merge lp:~mpontillo/maas/cli-as-sudo-uid-gid--bug-1598937--mpontillo--2.0 into lp:maas/2.0

[Mike Pontillo]

Backport fix for bug #1598937 from trunk.

[Mike Pontillo (mpontillo)]

Self-approve backport.

review: Approve

=== modified file 'src/maascli/config.py'
--- src/maascli/config.py	2015-12-01 18:12:59 +0000
+++ src/maascli/config.py	2016-07-13 00:19:55 +0000
@@ -1,4 +1,4 @@
-# Copyright 2012-2015 Canonical Ltd.  This software is licensed under the
+# Copyright 2012-2016 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Configuration abstractions for the MAAS CLI."""
@@ -16,6 +16,8 @@
 from os.path import expanduser
 import sqlite3
 
+from maascli import utils
+
 
 class ProfileConfig:
     """Store profile configurations in an sqlite3 database."""
@@ -61,6 +63,11 @@
                 " WHERE name = ?", (name,))
 
     @classmethod
+    def create_database(cls, dbpath):
+        # Initialise the database file with restrictive permissions.
+        os.close(os.open(dbpath, os.O_CREAT | os.O_APPEND, 0o600))
+
+    @classmethod
     @contextmanager
     def open(cls, dbpath=expanduser("~/.maascli.db")):
         """Load a profiles database.
@@ -71,9 +78,17 @@
         **Note** that this returns a context manager which will close the
         database on exit, saving if the exit is clean.
         """
-        # Initialise filename with restrictive permissions...
-        os.close(os.open(dbpath, os.O_CREAT | os.O_APPEND, 0o600))
-        # before opening it with sqlite.
+        # As the effective UID and GID of the user invoking `sudo` (if any)...
+        try:
+            with utils.sudo_gid(), utils.sudo_uid():
+                cls.create_database(dbpath)
+        except PermissionError:
+            # Creating the database might fail if $HOME is set to the current
+            # effective UID's $HOME, but we have permission to change the UID
+            # to one without permission to access $HOME. So try again without
+            # changing the GID/UID.
+            cls.create_database(dbpath)
+
         database = sqlite3.connect(dbpath)
         try:
             yield cls(database)
 
=== modified file 'src/maascli/tests/test_config.py'
--- src/maascli/tests/test_config.py	2015-12-01 18:12:59 +0000
+++ src/maascli/tests/test_config.py	2016-07-13 00:19:55 +0000
@@ -1,4 +1,4 @@
-# Copyright 2012-2015 Canonical Ltd.  This software is licensed under the
+# Copyright 2012-2016 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for `maascli.config`."""
@@ -6,10 +6,19 @@
 __all__ = []
 
 import contextlib
+from contextlib import contextmanager
 import os.path
 import sqlite3
+from unittest.mock import call
 
-from maascli import api
+from maascli import (
+    api,
+    utils,
+)
+from maastesting.matchers import (
+    MockCalledOnceWith,
+    MockCallsMatch,
+)
 from maastesting.testcase import MAASTestCase
 from twisted.python.filepath import FilePath
 
@@ -98,3 +107,42 @@
         with api.ProfileConfig.open(config_file):
             perms = FilePath(config_file).getPermissions()
             self.assertEqual("rw-r--r--", perms.shorthand())
+
+    def test_open_permissions_as_user_invoking_sudo(self):
+        # ProfileConfig.open() touches the database as user invoking `sudo`.
+
+        @contextmanager
+        def empty_context():
+            yield  # Do absolutely nothing.
+
+        self.patch_autospec(utils, "sudo_uid").side_effect = empty_context
+        self.patch_autospec(utils, "sudo_gid").side_effect = empty_context
+
+        config_file = os.path.join(self.make_dir(), "config")
+        with api.ProfileConfig.open(config_file):
+            # The sudo_uid and sudo_gid contexts have been used.
+            self.assertThat(utils.sudo_uid, MockCalledOnceWith())
+            self.assertThat(utils.sudo_gid, MockCalledOnceWith())
+
+    def test_open_permissions_as_user_invoking_sudo_retries_if_failed(self):
+        # ProfileConfig.open() touches the database as user invoking `sudo`,
+        # but falls back to the current UID if the operation fails.
+
+        @contextmanager
+        def empty_context():
+            yield  # Do absolutely nothing.
+
+        self.patch_autospec(utils, "sudo_uid").side_effect = empty_context
+        self.patch_autospec(utils, "sudo_gid").side_effect = empty_context
+        self.patch_autospec(api.ProfileConfig, "create_database")
+        api.ProfileConfig.create_database.side_effect = (
+            PermissionError,
+            None
+        )
+        config_file = os.path.join(self.make_dir(), "config")
+        with api.ProfileConfig.open(config_file):
+            self.assertThat(
+                api.ProfileConfig.create_database, MockCallsMatch(
+                    call(config_file),
+                    call(config_file)
+                ))
 
=== modified file 'src/maascli/tests/test_utils.py'
--- src/maascli/tests/test_utils.py	2016-05-12 19:07:37 +0000
+++ src/maascli/tests/test_utils.py	2016-07-13 00:19:55 +0000
@@ -8,14 +8,21 @@
 import collections
 import http.client
 import io
+import os
 import random
 from unittest.mock import sentinel
 
+from fixtures import EnvironmentVariable
 import httplib2
 from maascli import utils
 from maastesting.factory import factory
-from maastesting.matchers import MockCalledOnceWith
+from maastesting.matchers import (
+    MockCalledOnceWith,
+    MockCalledWith,
+    MockNotCalled,
+)
 from maastesting.testcase import MAASTestCase
+from testtools import ExpectedException
 from testtools.matchers import (
     AfterPreprocessing,
     Equals,
@@ -298,3 +305,71 @@
             b"Success.\n"
             b"Machine-readable output follows:\n" +
             response['content'] + b"\n", buf.getvalue())
+
+
+class TestSudoUID(MAASTestCase):
+    """Tests for `utils.sudo_uid`."""
+
+    def setUp(self):
+        super(TestSudoUID, self).setUp()
+        # Always ensure that SUDO_UID is not set in the environment.
+        self.useFixture(EnvironmentVariable("SUDO_UID"))
+        # We probably can't set EUID to anything we want in tests so we must
+        # capture calls that attempt to do so.
+        self.patch_autospec(utils, "seteuid")
+
+    def test_does_nothing_when_environ_not_set(self):
+        with utils.sudo_uid():
+            self.assertThat(utils.seteuid, MockNotCalled())
+        self.assertThat(utils.seteuid, MockNotCalled())
+
+    def test_sets_and_resets_euid(self):
+        original_euid = os.geteuid()
+        example_euid = original_euid + random.randrange(500, 1000)
+        self.useFixture(EnvironmentVariable("SUDO_UID", str(example_euid)))
+        with utils.sudo_uid():
+            self.assertThat(utils.seteuid, MockCalledOnceWith(example_euid))
+        self.assertThat(utils.seteuid, MockCalledWith(original_euid))
+
+    def test_sets_and_resets_euid_on_crash(self):
+        original_euid = os.geteuid()
+        example_euid = original_euid + random.randrange(500, 1000)
+        self.useFixture(EnvironmentVariable("SUDO_UID", str(example_euid)))
+        with ExpectedException(ZeroDivisionError):
+            with utils.sudo_uid():
+                0 / 0  # A very realistic example.
+        self.assertThat(utils.seteuid, MockCalledWith(original_euid))
+
+
+class TestSudoGID(MAASTestCase):
+    """Tests for `utils.sudo_gid`."""
+
+    def setUp(self):
+        super(TestSudoGID, self).setUp()
+        # Always ensure that SUDO_GID is not set in the environment.
+        self.useFixture(EnvironmentVariable("SUDO_GID"))
+        # We probably can't set EGID to anything we want in tests so we must
+        # capture calls that attempt to do so.
+        self.patch_autospec(utils, "setegid")
+
+    def test_does_nothing_when_environ_not_set(self):
+        with utils.sudo_gid():
+            self.assertThat(utils.setegid, MockNotCalled())
+        self.assertThat(utils.setegid, MockNotCalled())
+
+    def test_sets_and_resets_egid(self):
+        original_egid = os.getegid()
+        example_egid = original_egid + random.randrange(500, 1000)
+        self.useFixture(EnvironmentVariable("SUDO_GID", str(example_egid)))
+        with utils.sudo_gid():
+            self.assertThat(utils.setegid, MockCalledOnceWith(example_egid))
+        self.assertThat(utils.setegid, MockCalledWith(original_egid))
+
+    def test_sets_and_resets_egid_on_crash(self):
+        original_egid = os.getegid()
+        example_egid = original_egid + random.randrange(500, 1000)
+        self.useFixture(EnvironmentVariable("SUDO_GID", str(example_egid)))
+        with ExpectedException(ZeroDivisionError):
+            with utils.sudo_gid():
+                0 / 0  # A very realistic example.
+        self.assertThat(utils.setegid, MockCalledWith(original_egid))
 
=== modified file 'src/maascli/utils.py'
--- src/maascli/utils.py	2016-03-28 13:54:47 +0000
+++ src/maascli/utils.py	2016-07-13 00:19:55 +0000
@@ -13,8 +13,11 @@
     "print_response_content",
     "print_response_headers",
     "safe_name",
+    "sudo_gid",
+    "sudo_uid",
 ]
 
+from contextlib import contextmanager
 from email.message import Message
 from functools import partial
 from inspect import (
@@ -22,6 +25,11 @@
     getdoc,
 )
 import io
+import os
+from os import (
+    setegid,
+    seteuid,
+)
 import re
 import sys
 from urllib.parse import urlparse
@@ -202,3 +210,35 @@
     print(file=file)
     print_response_headers(response, file=file)
     print(file=file)
+
+
+@contextmanager
+def sudo_uid():
+    """Context to revert effective UID to that of the user invoking `sudo`."""
+    try:
+        sudo_uid = os.environ["SUDO_UID"]
+    except KeyError:
+        yield  # Not running under sudo.
+    else:
+        orig_euid = os.geteuid()
+        seteuid(int(sudo_uid))
+        try:
+            yield
+        finally:
+            seteuid(orig_euid)
+
+
+@contextmanager
+def sudo_gid():
+    """Context to revert effective GID to that of the user invoking `sudo`."""
+    try:
+        sudo_gid = os.environ["SUDO_GID"]
+    except KeyError:
+        yield  # Not running under sudo.
+    else:
+        orig_egid = os.getegid()
+        setegid(int(sudo_gid))
+        try:
+            yield
+        finally:
+            setegid(orig_egid)