Support sudoers in the "usr merge" location

[rjschwei]

Proposed Commit Message

When sudo is compiled with the secure-path option and the configuration file exists in /usr/etc and not in /etc take the configuration in /usr/etc into account. If the desired include directive exists in the system (/usr/etc ) configuration there is nothing to do if there is no file in etc.

Additional Context

Test Steps

Checklist

• [x ] My code follows the process laid out in the documentation
• [ x] I have updated or added any unit tests accordingly
• I have updated or added any documentation accordingly

Merge type

• Squash merge using "Proposed Commit Message"
• Rebase and merge unique commits. Requires commit messages per-commit each referencing the pull request number (#<PR_NUM>)

[rjschwei]

@blackboxsw need some help with the test please, I do not understand why I am triggering a permission error in the mock for the file write. Thanks

[blackboxsw]

Looks pretty good for handling the case of /usr/etc/sudoers vs an absent /etc/sudoers file. Some general questions I have are:

• Is the a changeset represented in upstream sudo project that points to this behavior and/or /usr/etc/sudoers overrides?
• what documentation tells us we should ignore /usr/etc/sudoers when /etc/sudoers exits?
• What should be the behavior on a system with both /etc/sudoers.d and /usr/etc/sudoers.d? Do all the sudo config parts.d directory get aggregated?

[blackboxsw]

Will there be a case where /usr/etc/sudoers exists bit /etc/sudoers does not? It seems this is possible given this read-only FS note in SUSE docs which talk about visudo writing to /etc/sudoers if /usr/etc/sudoers exists but /etc/sudoers does not.

Do we want to completely ignore /usr/etc/sudoers in any environment which already has an /etc/sudoers file? There is no active merge of /usr/etc/sudoers content with primary /etc/sudoers anywhere.

[rjschwei]

Will there be a case where /usr/etc/sudoers exists bit /etc/sudoers does not? It seems this is possible given this read-only FS note in SUSE docs which talk about visudo writing to /etc/sudoers if /usr/etc/sudoers exists but /etc/sudoers does not.

Yes, in newer SUSE distributions /etc/sudoers will not exist by default.

Do we want to completely ignore /usr/etc/sudoers in any environment which already has an /etc/sudoers file? There is no active merge of /usr/etc/sudoers content with primary /etc/sudoers anywhere.

If /etc/sudoers exists the underlying premise is that the user (sysadmin) has made the modifications they want to have in place. Should the include directive be missing we add it to this file to match the current behavior. If /etc/usr/sudoers exists and contains the include directive and we would not add the include directive to an existing /etc/sudoers we would change the behavior compared to the current implementation because the include files would be processed at a different point in time.

[blackboxsw]

In the case where /etc/sudoers doesn't exist and /usr/etc/sudoers does exist . We fall into this conditional where we now create a root read-only /etc/sudoers file with a copy of /usr/etc/soduers (presumably the sudoers path of the read-only filesystem). At this point, permissions of 0o440 will disallow our calling of append_file below as a non-root user that is executing the unit tests. The reason other unit tests pass is because those unit tests actually performed the util.write_file("/etc/sudeors") with default mode 0o644 leaving the unittest able to append_file.

[blackboxsw]

Here is a unit-test workaround that avoids a non-root test-user getting hit by the read-only permission check on the test-created /etc/sudoers

diff --git a/cloudinit/distros/__init__.py b/cloudinit/distros/__init__.py
index ab656ab5a..63b50b2fc 100644
--- a/cloudinit/distros/__init__.py
+++ b/cloudinit/distros/__init__.py
@@ -1048,6 +1048,11 @@ class Distro(persistence.CloudInitPickleMixin, metaclass=abc.ABCMeta):
                     util.write_file(admin_sudo_base, sudoers_contents, 0o440)
                 else:
                     if sudoers_base != admin_sudo_base:
+                        LOG.debug(
+                            "Copying content from %s to %s",
+                            sudoers_base,
+                            admin_sudo_base,
+                        )
                         util.write_file(
                             admin_sudo_base, sudoers_contents, 0o440)
                     lines = [
diff --git a/tests/unittests/distros/test__init__.py b/tests/unittests/distros/test__init__.py
index e762f79f6..aa682acc5 100644
--- a/tests/unittests/distros/test__init__.py
+++ b/tests/unittests/distros/test__init__.py
@@ -251,9 +251,15 @@ class TestGenericDistro(helpers.FilesystemMockingTestCase):
         self.patchOS(self.tmp)
         self.patchUtils(self.tmp)
         util.write_file("/usr/etc/sudoers", "josh, josh\n")
-        d.ensure_sudo_dir("/b")
+        # This test creates missing /etc/sudoers with mode=0o440.
+        # Avoid append_file which raises a permissions error on 0o440.
+        with mock.patch("cloudinit.distros.util.append_file") as append:
+            d.ensure_sudo_dir("/b")
+        path, appended_content = append.call_args_list[0][0]
+        self.assertEqual("/etc/sudoers",  path)
+        self.assertRegex(appended_content, r"#includedir \/b")
+
         contents = util.load_text_file("/etc/sudoers")
-        self.assertIn("includedir /b", contents)
         self.assertTrue(os.path.isdir("/b"))
         self.assertIn("josh", contents)
         self.assertEqual(2, contents.count("josh"))

[blackboxsw]

Let's log what we've done here, otherwise our write_files logging won't really tell us where we got the contents. "Copying content from /usr/etc/sudoers to /etc/sudoers"

[rjschwei]

OK, thanks for the questions, made me realize that I had a logic error. There is no need to copy the content from /usr/etc/sudoers if it exists. If the "secure path" file does not have the desired include directive and the "sysadmin location", i.e. /etc/sudoers does not exist we simply write the include directive to /etc/sudoers . If /etc/sudoers exists but does not have the include directive we append there to preserve the current behavior.

[rjschwei]

Looks pretty good for handling the case of /usr/etc/sudoers vs an absent /etc/sudoers file. Some general questions I have are:

• Is the a changeset represented in upstream sudo project that points to this behavior and/or /usr/etc/sudoers overrides?

Not that I can find. The existence of /usr/etc appears to be quite old and can be when building the upstream sources with --enable-secure-path. For SUSE distributions we set the configuration explicitly with

%if %{defined _distconfdir}
    --prefix=/usr \
    --sysconfdir=%{_distconfdir} \
    --enable-adminconf=%{_sysconfdir} \
%endif

The condition on defined _distconfdir exists to differentiate the build between distributions that have /usr/etc and distributions that do not.

https://build.opensuse.org/projects/openSUSE:Factory/packages/sudo/files/sudo.spec?expand=1

• what documentation tells us we should ignore /usr/etc/sudoers when /etc/sudoers exits?

I changed that in the current update. I twisted myself into a knot with the first go around. The logic is more straight forwrad now, IMHO.

• What should be the behavior on a system with both /etc/sudoers.d and /usr/etc/sudoers.d? Do all the sudo config parts.d directory get aggregated?

The default SUSE configuration is to include /usr/etc/sudoers.d and then /etc/sudoers.d. So the answer is yes they do. The updated logic makes sure that /etc/sudoers.d is included last if no include directive exists in either /etc/sudoers or /usr/etc/sudoers preserving the current behavior.

[github-actions]

Hello! Thank you for this proposed change to cloud-init. This pull request is now marked as stale as it has not seen any activity in 14 days. If no activity occurs within the next 7 days, this pull request will automatically close.

If you are waiting for code review and you are seeing this message, apologies! Please reply, tagging TheRealFalcon, and he will ensure that someone takes a look soon.

(If the pull request is closed and you would like to continue working on it, please do tag TheRealFalcon to reopen it.)

[rjschwei]

@blackboxsw I think I addressed the requested changes. Anything else?

[blackboxsw]

Thank you @rjschwei this is much simpler and straight-forward.

I've added a couple of supplemental log checks to unittests here. but looks good to me.

commit 43d29f1350057ab60c992c340eda8cf2e0f227a2 (HEAD -> usrMerge)
Author: Chad Smith <[email protected]>
Date:   Wed May 29 21:27:20 2024 -0600

    test log additions

diff --git a/tests/unittests/distros/test__init__.py b/tests/unittests/distros/test__init__.py
index 6ce3d25bd..39583b137 100644
--- a/tests/unittests/distros/test__init__.py
+++ b/tests/unittests/distros/test__init__.py
@@ -55,6 +55,8 @@ gapmi = distros._get_arch_package_mirror_info
 
 
 class TestGenericDistro(helpers.FilesystemMockingTestCase):
+    with_logs = True
+
     def setUp(self):
         super(TestGenericDistro, self).setUp()
         # Make a temp directoy for tests to use.
@@ -231,6 +233,9 @@ class TestGenericDistro(helpers.FilesystemMockingTestCase):
         self.assertTrue(os.path.isdir("/b"))
         self.assertIn("josh", contents)
         self.assertEqual(2, contents.count("josh"))
+        self.assertIn(
+            "Added '#includedir /b' to /etc/sudoers", self.logs.getvalue()
+        )
 
     def test_sudoers_ensure_append_sudoer_file(self):
         cls = distros.fetch("ubuntu")
@@ -257,8 +262,11 @@ class TestGenericDistro(helpers.FilesystemMockingTestCase):
         self.assertEqual(2, contents.count("josh"))
         self.assertIn("includedir /b", contents)
         self.assertTrue(os.path.isdir("/b"))
+        self.assertIn(
+            "Using content from '/usr/etc/sudoers", self.logs.getvalue()
+        )
 
-    def test_usr_sudoers_ensure_no_etc_creat(self):
+    def test_usr_sudoers_ensure_no_etc_create_when_include_in_usr_etc(self):
         cls = distros.fetch("ubuntu")
         d = cls("ubuntu", {}, None)
         self.patchOS(self.tmp)

In a followup PR this cycle we'll be converting all this FilesystemMocking tests to pytest so look for that in the future.