sailesh1993 / rpms / cloud-init

Forked from rpms/cloud-init a year ago
Clone
d9e4dd
From f9564bd4477782e8cffe4be1d3c31c0338fb03b1 Mon Sep 17 00:00:00 2001
d9e4dd
From: Eduardo Otubo <otubo@redhat.com>
d9e4dd
Date: Mon, 5 Jul 2021 14:07:21 +0200
d9e4dd
Subject: [PATCH 1/2] write passwords only to serial console, lock down
d9e4dd
 cloud-init-output.log (#847)
d9e4dd
d9e4dd
RH-Author: Eduardo Otubo <otubo@redhat.com>
d9e4dd
RH-MergeRequest: 4: write passwords only to serial console, lock down cloud-init-output.log (#847)
d9e4dd
RH-Commit: [1/1] 7543b3458c01ea988e987336d84510157c00390d (otubo/cloud-init-src)
d9e4dd
RH-Bugzilla: 1945892
d9e4dd
RH-Acked-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
d9e4dd
RH-Acked-by: Miroslav Rezanina <mrezanin@redhat.com>
d9e4dd
RH-Acked-by: Mohamed Gamal Morsy <mmorsy@redhat.com>
d9e4dd
d9e4dd
commit b794d426b9ab43ea9d6371477466070d86e10668
d9e4dd
Author: Daniel Watkins <oddbloke@ubuntu.com>
d9e4dd
Date:   Fri Mar 19 10:06:42 2021 -0400
d9e4dd
d9e4dd
    write passwords only to serial console, lock down cloud-init-output.log (#847)
d9e4dd
d9e4dd
    Prior to this commit, when a user specified configuration which would
d9e4dd
    generate random passwords for users, cloud-init would cause those
d9e4dd
    passwords to be written to the serial console by emitting them on
d9e4dd
    stderr.  In the default configuration, any stdout or stderr emitted by
d9e4dd
    cloud-init is also written to `/var/log/cloud-init-output.log`.  This
d9e4dd
    file is world-readable, meaning that those randomly-generated passwords
d9e4dd
    were available to be read by any user with access to the system.  This
d9e4dd
    presents an obvious security issue.
d9e4dd
d9e4dd
    This commit responds to this issue in two ways:
d9e4dd
d9e4dd
    * We address the direct issue by moving from writing the passwords to
d9e4dd
      sys.stderr to writing them directly to /dev/console (via
d9e4dd
      util.multi_log); this means that the passwords will never end up in
d9e4dd
      cloud-init-output.log
d9e4dd
    * To avoid future issues like this, we also modify the logging code so
d9e4dd
      that any files created in a log sink subprocess will only be
d9e4dd
      owner/group readable and, if it exists, will be owned by the adm
d9e4dd
      group.  This results in `/var/log/cloud-init-output.log` no longer
d9e4dd
      being world-readable, meaning that if there are other parts of the
d9e4dd
      codebase that are emitting sensitive data intended for the serial
d9e4dd
      console, that data is no longer available to all users of the system.
d9e4dd
d9e4dd
    LP: #1918303
d9e4dd
d9e4dd
Signed-off-by: Eduardo Otubo <otubo@redhat.com>
d9e4dd
Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com>
d9e4dd
---
d9e4dd
 cloudinit/config/cc_set_passwords.py          |  5 +-
d9e4dd
 cloudinit/config/tests/test_set_passwords.py  | 40 +++++++++----
d9e4dd
 cloudinit/tests/test_util.py                  | 56 +++++++++++++++++++
d9e4dd
 cloudinit/util.py                             | 38 +++++++++++--
d9e4dd
 .../modules/test_set_password.py              | 24 ++++++++
d9e4dd
 tests/integration_tests/test_logging.py       | 22 ++++++++
d9e4dd
 tests/unittests/test_util.py                  |  4 ++
d9e4dd
 7 files changed, 173 insertions(+), 16 deletions(-)
d9e4dd
 create mode 100644 tests/integration_tests/test_logging.py
d9e4dd
d9e4dd
diff --git a/cloudinit/config/cc_set_passwords.py b/cloudinit/config/cc_set_passwords.py
d9e4dd
index d6b5682d..433de751 100755
d9e4dd
--- a/cloudinit/config/cc_set_passwords.py
d9e4dd
+++ b/cloudinit/config/cc_set_passwords.py
d9e4dd
@@ -78,7 +78,6 @@ password.
d9e4dd
 """
d9e4dd
 
d9e4dd
 import re
d9e4dd
-import sys
d9e4dd
 
d9e4dd
 from cloudinit.distros import ug_util
d9e4dd
 from cloudinit import log as logging
d9e4dd
@@ -214,7 +213,9 @@ def handle(_name, cfg, cloud, log, args):
d9e4dd
         if len(randlist):
d9e4dd
             blurb = ("Set the following 'random' passwords\n",
d9e4dd
                      '\n'.join(randlist))
d9e4dd
-            sys.stderr.write("%s\n%s\n" % blurb)
d9e4dd
+            util.multi_log(
d9e4dd
+                "%s\n%s\n" % blurb, stderr=False, fallback_to_stdout=False
d9e4dd
+            )
d9e4dd
 
d9e4dd
         if expire:
d9e4dd
             expired_users = []
d9e4dd
diff --git a/cloudinit/config/tests/test_set_passwords.py b/cloudinit/config/tests/test_set_passwords.py
d9e4dd
index daa1ef51..bbe2ee8f 100644
d9e4dd
--- a/cloudinit/config/tests/test_set_passwords.py
d9e4dd
+++ b/cloudinit/config/tests/test_set_passwords.py
d9e4dd
@@ -74,10 +74,6 @@ class TestSetPasswordsHandle(CiTestCase):
d9e4dd
 
d9e4dd
     with_logs = True
d9e4dd
 
d9e4dd
-    def setUp(self):
d9e4dd
-        super(TestSetPasswordsHandle, self).setUp()
d9e4dd
-        self.add_patch('cloudinit.config.cc_set_passwords.sys.stderr', 'm_err')
d9e4dd
-
d9e4dd
     def test_handle_on_empty_config(self, *args):
d9e4dd
         """handle logs that no password has changed when config is empty."""
d9e4dd
         cloud = self.tmp_cloud(distro='ubuntu')
d9e4dd
@@ -129,10 +125,12 @@ class TestSetPasswordsHandle(CiTestCase):
d9e4dd
             mock.call(['pw', 'usermod', 'ubuntu', '-p', '01-Jan-1970'])],
d9e4dd
             m_subp.call_args_list)
d9e4dd
 
d9e4dd
+    @mock.patch(MODPATH + "util.multi_log")
d9e4dd
     @mock.patch(MODPATH + "util.is_BSD")
d9e4dd
     @mock.patch(MODPATH + "subp.subp")
d9e4dd
-    def test_handle_on_chpasswd_list_creates_random_passwords(self, m_subp,
d9e4dd
-                                                              m_is_bsd):
d9e4dd
+    def test_handle_on_chpasswd_list_creates_random_passwords(
d9e4dd
+        self, m_subp, m_is_bsd, m_multi_log
d9e4dd
+    ):
d9e4dd
         """handle parses command set random passwords."""
d9e4dd
         m_is_bsd.return_value = False
d9e4dd
         cloud = self.tmp_cloud(distro='ubuntu')
d9e4dd
@@ -146,10 +144,32 @@ class TestSetPasswordsHandle(CiTestCase):
d9e4dd
         self.assertIn(
d9e4dd
             'DEBUG: Handling input for chpasswd as list.',
d9e4dd
             self.logs.getvalue())
d9e4dd
-        self.assertNotEqual(
d9e4dd
-            [mock.call(['chpasswd'],
d9e4dd
-             '\n'.join(valid_random_pwds) + '\n')],
d9e4dd
-            m_subp.call_args_list)
d9e4dd
+
d9e4dd
+        self.assertEqual(1, m_subp.call_count)
d9e4dd
+        args, _kwargs = m_subp.call_args
d9e4dd
+        self.assertEqual(["chpasswd"], args[0])
d9e4dd
+
d9e4dd
+        stdin = args[1]
d9e4dd
+        user_pass = {
d9e4dd
+            user: password
d9e4dd
+            for user, password
d9e4dd
+            in (line.split(":") for line in stdin.splitlines())
d9e4dd
+        }
d9e4dd
+
d9e4dd
+        self.assertEqual(1, m_multi_log.call_count)
d9e4dd
+        self.assertEqual(
d9e4dd
+            mock.call(mock.ANY, stderr=False, fallback_to_stdout=False),
d9e4dd
+            m_multi_log.call_args
d9e4dd
+        )
d9e4dd
+
d9e4dd
+        self.assertEqual(set(["root", "ubuntu"]), set(user_pass.keys()))
d9e4dd
+        written_lines = m_multi_log.call_args[0][0].splitlines()
d9e4dd
+        for password in user_pass.values():
d9e4dd
+            for line in written_lines:
d9e4dd
+                if password in line:
d9e4dd
+                    break
d9e4dd
+            else:
d9e4dd
+                self.fail("Password not emitted to console")
d9e4dd
 
d9e4dd
 
d9e4dd
 # vi: ts=4 expandtab
d9e4dd
diff --git a/cloudinit/tests/test_util.py b/cloudinit/tests/test_util.py
d9e4dd
index b7a302f1..e811917e 100644
d9e4dd
--- a/cloudinit/tests/test_util.py
d9e4dd
+++ b/cloudinit/tests/test_util.py
d9e4dd
@@ -851,4 +851,60 @@ class TestEnsureFile:
d9e4dd
         assert "ab" == kwargs["omode"]
d9e4dd
 
d9e4dd
 
d9e4dd
+@mock.patch("cloudinit.util.grp.getgrnam")
d9e4dd
+@mock.patch("cloudinit.util.os.setgid")
d9e4dd
+@mock.patch("cloudinit.util.os.umask")
d9e4dd
+class TestRedirectOutputPreexecFn:
d9e4dd
+    """This tests specifically the preexec_fn used in redirect_output."""
d9e4dd
+
d9e4dd
+    @pytest.fixture(params=["outfmt", "errfmt"])
d9e4dd
+    def preexec_fn(self, request):
d9e4dd
+        """A fixture to gather the preexec_fn used by redirect_output.
d9e4dd
+
d9e4dd
+        This enables simpler direct testing of it, and parameterises any tests
d9e4dd
+        using it to cover both the stdout and stderr code paths.
d9e4dd
+        """
d9e4dd
+        test_string = "| piped output to invoke subprocess"
d9e4dd
+        if request.param == "outfmt":
d9e4dd
+            args = (test_string, None)
d9e4dd
+        elif request.param == "errfmt":
d9e4dd
+            args = (None, test_string)
d9e4dd
+        with mock.patch("cloudinit.util.subprocess.Popen") as m_popen:
d9e4dd
+            util.redirect_output(*args)
d9e4dd
+
d9e4dd
+        assert 1 == m_popen.call_count
d9e4dd
+        _args, kwargs = m_popen.call_args
d9e4dd
+        assert "preexec_fn" in kwargs, "preexec_fn not passed to Popen"
d9e4dd
+        return kwargs["preexec_fn"]
d9e4dd
+
d9e4dd
+    def test_preexec_fn_sets_umask(
d9e4dd
+        self, m_os_umask, _m_setgid, _m_getgrnam, preexec_fn
d9e4dd
+    ):
d9e4dd
+        """preexec_fn should set a mask that avoids world-readable files."""
d9e4dd
+        preexec_fn()
d9e4dd
+
d9e4dd
+        assert [mock.call(0o037)] == m_os_umask.call_args_list
d9e4dd
+
d9e4dd
+    def test_preexec_fn_sets_group_id_if_adm_group_present(
d9e4dd
+        self, _m_os_umask, m_setgid, m_getgrnam, preexec_fn
d9e4dd
+    ):
d9e4dd
+        """We should setgrp to adm if present, so files are owned by them."""
d9e4dd
+        fake_group = mock.Mock(gr_gid=mock.sentinel.gr_gid)
d9e4dd
+        m_getgrnam.return_value = fake_group
d9e4dd
+
d9e4dd
+        preexec_fn()
d9e4dd
+
d9e4dd
+        assert [mock.call("adm")] == m_getgrnam.call_args_list
d9e4dd
+        assert [mock.call(mock.sentinel.gr_gid)] == m_setgid.call_args_list
d9e4dd
+
d9e4dd
+    def test_preexec_fn_handles_absent_adm_group_gracefully(
d9e4dd
+        self, _m_os_umask, m_setgid, m_getgrnam, preexec_fn
d9e4dd
+    ):
d9e4dd
+        """We should handle an absent adm group gracefully."""
d9e4dd
+        m_getgrnam.side_effect = KeyError("getgrnam(): name not found: 'adm'")
d9e4dd
+
d9e4dd
+        preexec_fn()
d9e4dd
+
d9e4dd
+        assert 0 == m_setgid.call_count
d9e4dd
+
d9e4dd
 # vi: ts=4 expandtab
d9e4dd
diff --git a/cloudinit/util.py b/cloudinit/util.py
d9e4dd
index 769f3425..4e0a72db 100644
d9e4dd
--- a/cloudinit/util.py
d9e4dd
+++ b/cloudinit/util.py
d9e4dd
@@ -359,7 +359,7 @@ def find_modules(root_dir):
d9e4dd
 
d9e4dd
 
d9e4dd
 def multi_log(text, console=True, stderr=True,
d9e4dd
-              log=None, log_level=logging.DEBUG):
d9e4dd
+              log=None, log_level=logging.DEBUG, fallback_to_stdout=True):
d9e4dd
     if stderr:
d9e4dd
         sys.stderr.write(text)
d9e4dd
     if console:
d9e4dd
@@ -368,7 +368,7 @@ def multi_log(text, console=True, stderr=True,
d9e4dd
             with open(conpath, 'w') as wfh:
d9e4dd
                 wfh.write(text)
d9e4dd
                 wfh.flush()
d9e4dd
-        else:
d9e4dd
+        elif fallback_to_stdout:
d9e4dd
             # A container may lack /dev/console (arguably a container bug).  If
d9e4dd
             # it does not exist, then write output to stdout.  this will result
d9e4dd
             # in duplicate stderr and stdout messages if stderr was True.
d9e4dd
@@ -623,6 +623,26 @@ def redirect_output(outfmt, errfmt, o_out=None, o_err=None):
d9e4dd
     if not o_err:
d9e4dd
         o_err = sys.stderr
d9e4dd
 
d9e4dd
+    # pylint: disable=subprocess-popen-preexec-fn
d9e4dd
+    def set_subprocess_umask_and_gid():
d9e4dd
+        """Reconfigure umask and group ID to create output files securely.
d9e4dd
+
d9e4dd
+        This is passed to subprocess.Popen as preexec_fn, so it is executed in
d9e4dd
+        the context of the newly-created process.  It:
d9e4dd
+
d9e4dd
+        * sets the umask of the process so created files aren't world-readable
d9e4dd
+        * if an adm group exists in the system, sets that as the process' GID
d9e4dd
+          (so that the created file(s) are owned by root:adm)
d9e4dd
+        """
d9e4dd
+        os.umask(0o037)
d9e4dd
+        try:
d9e4dd
+            group_id = grp.getgrnam("adm").gr_gid
d9e4dd
+        except KeyError:
d9e4dd
+            # No adm group, don't set a group
d9e4dd
+            pass
d9e4dd
+        else:
d9e4dd
+            os.setgid(group_id)
d9e4dd
+
d9e4dd
     if outfmt:
d9e4dd
         LOG.debug("Redirecting %s to %s", o_out, outfmt)
d9e4dd
         (mode, arg) = outfmt.split(" ", 1)
d9e4dd
@@ -632,7 +652,12 @@ def redirect_output(outfmt, errfmt, o_out=None, o_err=None):
d9e4dd
                 owith = "wb"
d9e4dd
             new_fp = open(arg, owith)
d9e4dd
         elif mode == "|":
d9e4dd
-            proc = subprocess.Popen(arg, shell=True, stdin=subprocess.PIPE)
d9e4dd
+            proc = subprocess.Popen(
d9e4dd
+                arg,
d9e4dd
+                shell=True,
d9e4dd
+                stdin=subprocess.PIPE,
d9e4dd
+                preexec_fn=set_subprocess_umask_and_gid,
d9e4dd
+            )
d9e4dd
             new_fp = proc.stdin
d9e4dd
         else:
d9e4dd
             raise TypeError("Invalid type for output format: %s" % outfmt)
d9e4dd
@@ -654,7 +679,12 @@ def redirect_output(outfmt, errfmt, o_out=None, o_err=None):
d9e4dd
                 owith = "wb"
d9e4dd
             new_fp = open(arg, owith)
d9e4dd
         elif mode == "|":
d9e4dd
-            proc = subprocess.Popen(arg, shell=True, stdin=subprocess.PIPE)
d9e4dd
+            proc = subprocess.Popen(
d9e4dd
+                arg,
d9e4dd
+                shell=True,
d9e4dd
+                stdin=subprocess.PIPE,
d9e4dd
+                preexec_fn=set_subprocess_umask_and_gid,
d9e4dd
+            )
d9e4dd
             new_fp = proc.stdin
d9e4dd
         else:
d9e4dd
             raise TypeError("Invalid type for error format: %s" % errfmt)
d9e4dd
diff --git a/tests/integration_tests/modules/test_set_password.py b/tests/integration_tests/modules/test_set_password.py
d9e4dd
index b13f76fb..d7cf91a5 100644
d9e4dd
--- a/tests/integration_tests/modules/test_set_password.py
d9e4dd
+++ b/tests/integration_tests/modules/test_set_password.py
d9e4dd
@@ -116,6 +116,30 @@ class Mixin:
d9e4dd
         # Which are not the same
d9e4dd
         assert shadow_users["harry"] != shadow_users["dick"]
d9e4dd
 
d9e4dd
+    def test_random_passwords_not_stored_in_cloud_init_output_log(
d9e4dd
+        self, class_client
d9e4dd
+    ):
d9e4dd
+        """We should not emit passwords to the in-instance log file.
d9e4dd
+
d9e4dd
+        LP: #1918303
d9e4dd
+        """
d9e4dd
+        cloud_init_output = class_client.read_from_file(
d9e4dd
+            "/var/log/cloud-init-output.log"
d9e4dd
+        )
d9e4dd
+        assert "dick:" not in cloud_init_output
d9e4dd
+        assert "harry:" not in cloud_init_output
d9e4dd
+
d9e4dd
+    def test_random_passwords_emitted_to_serial_console(self, class_client):
d9e4dd
+        """We should emit passwords to the serial console. (LP: #1918303)"""
d9e4dd
+        try:
d9e4dd
+            console_log = class_client.instance.console_log()
d9e4dd
+        except NotImplementedError:
d9e4dd
+            # Assume that an exception here means that we can't use the console
d9e4dd
+            # log
d9e4dd
+            pytest.skip("NotImplementedError when requesting console log")
d9e4dd
+        assert "dick:" in console_log
d9e4dd
+        assert "harry:" in console_log
d9e4dd
+
d9e4dd
     def test_explicit_password_set_correctly(self, class_client):
d9e4dd
         """Test that an explicitly-specified password is set correctly."""
d9e4dd
         shadow_users, _ = self._fetch_and_parse_etc_shadow(class_client)
d9e4dd
diff --git a/tests/integration_tests/test_logging.py b/tests/integration_tests/test_logging.py
d9e4dd
new file mode 100644
d9e4dd
index 00000000..b31a0434
d9e4dd
--- /dev/null
d9e4dd
+++ b/tests/integration_tests/test_logging.py
d9e4dd
@@ -0,0 +1,22 @@
d9e4dd
+"""Integration tests relating to cloud-init's logging."""
d9e4dd
+
d9e4dd
+
d9e4dd
+class TestVarLogCloudInitOutput:
d9e4dd
+    """Integration tests relating to /var/log/cloud-init-output.log."""
d9e4dd
+
d9e4dd
+    def test_var_log_cloud_init_output_not_world_readable(self, client):
d9e4dd
+        """
d9e4dd
+        The log can contain sensitive data, it shouldn't be world-readable.
d9e4dd
+
d9e4dd
+        LP: #1918303
d9e4dd
+        """
d9e4dd
+        # Check the file exists
d9e4dd
+        assert client.execute("test -f /var/log/cloud-init-output.log").ok
d9e4dd
+
d9e4dd
+        # Check its permissions are as we expect
d9e4dd
+        perms, user, group = client.execute(
d9e4dd
+            "stat -c %a:%U:%G /var/log/cloud-init-output.log"
d9e4dd
+        ).split(":")
d9e4dd
+        assert "640" == perms
d9e4dd
+        assert "root" == user
d9e4dd
+        assert "adm" == group
d9e4dd
diff --git a/tests/unittests/test_util.py b/tests/unittests/test_util.py
d9e4dd
index 857629f1..e5292001 100644
d9e4dd
--- a/tests/unittests/test_util.py
d9e4dd
+++ b/tests/unittests/test_util.py
d9e4dd
@@ -572,6 +572,10 @@ class TestMultiLog(helpers.FilesystemMockingTestCase):
d9e4dd
         util.multi_log(logged_string)
d9e4dd
         self.assertEqual(logged_string, self.stdout.getvalue())
d9e4dd
 
d9e4dd
+    def test_logs_dont_go_to_stdout_if_fallback_to_stdout_is_false(self):
d9e4dd
+        util.multi_log('something', fallback_to_stdout=False)
d9e4dd
+        self.assertEqual('', self.stdout.getvalue())
d9e4dd
+
d9e4dd
     def test_logs_go_to_log_if_given(self):
d9e4dd
         log = mock.MagicMock()
d9e4dd
         logged_string = 'something very important'
d9e4dd
-- 
d9e4dd
2.27.0
d9e4dd