[3006.x] Preserve ownership on log rotation

changelog/65288.fixed.md

@@ -0,0 +1 @@
+Preserve ownership on log rotation

pkg/common/logrotate/salt-common

@@ -4,7 +4,7 @@
 	rotate 7
 	compress
 	notifempty
-	create 0640 salt salt
+	create 0640
 }
 
 /var/log/salt/minion {
@@ -13,6 +13,7 @@
 	rotate 7
 	compress
 	notifempty
+	create 0640
 }
 
 /var/log/salt/key {
@@ -21,7 +22,7 @@
 	rotate 7
 	compress
 	notifempty
-	create 0640 salt salt
+	create 0640
 }
 
 /var/log/salt/api {
@@ -30,7 +31,7 @@
 	rotate 7
 	compress
 	notifempty
-	create 0640 salt salt
+	create 0640
 }
 
 /var/log/salt/syndic {
@@ -39,6 +40,7 @@
 	rotate 7
 	compress
 	notifempty
+	create 0640
 }
 
 /var/log/salt/proxy {
@@ -47,4 +49,5 @@
 	rotate 7
 	compress
 	notifempty
+	create 0640
 }

pkg/tests/integration/test_salt_user.py

@@ -6,6 +6,7 @@
 import packaging.version
 import psutil
 import pytest
+from saltfactories.utils.tempfiles import temp_directory
 
 pytestmark = [
     pytest.mark.skip_on_windows,
@@ -135,9 +136,9 @@ def test_pkg_paths(
     Test package paths ownership
     """
     if packaging.version.parse(install_salt.version) <= packaging.version.parse(
-        "3006.2"
+        "3006.4"
     ):
-        pytest.skip("Package path ownership was changed in salt 3006.3")
+        pytest.skip("Package path ownership was changed in salt 3006.4")
     salt_user_subdirs = []
     for _path in pkg_paths:
         pkg_path = pathlib.Path(_path)
@@ -170,3 +171,189 @@ def test_pkg_paths(
                     else:
                         assert file_path.owner() == "root"
                         assert file_path.group() == "root"
+
+
+@pytest.mark.skip_if_binaries_missing("logrotate")
+def test_paths_log_rotation(
+    salt_master, salt_minion, salt_call_cli, install_salt, test_account
+):
+    """
+    Test the correct ownership is assigned when log rotation occurs
+    Change the user in the Salt Master, chage ownership, force logrotation
+    Check ownership and premissions.
+    Assumes test_pkg_paths successful
+    """
+    if packaging.version.parse(install_salt.version) <= packaging.version.parse(
+        "3006.4"
+    ):
+        pytest.skip("Package path ownership was changed in salt 3006.4")
+
+    if install_salt.distro_id not in ("centos", "redhat", "amzn", "fedora"):
+        pytest.skip(
+            "Only tests RedHat family packages till logrotation paths are resolved on Ubuntu/Debian, see issue 65231"
+        )
+
+    # check that the salt_master is running
+    assert salt_master.is_running()
+    match = False
+    for proc in psutil.Process(salt_master.pid).children():
+        assert proc.username() == "salt"
+        match = True
+
+    assert match
+
+    # Paths created by package installs with adjustment for current conf_dir /etc/salt
+    log_pkg_paths = [
+        install_salt.conf_dir,  # "bkup0"
+        "/var/cache/salt",  # "bkup1"
+        "/var/log/salt",  # "bkup2"
+        "/var/run/salt",  # "bkup3"
+        "/opt/saltstack/salt",  # "bkup4"
+    ]
+
+    # backup those about to change
+    bkup_count = 0
+    bkup_count_max = 5
+    with temp_directory("bkup0") as temp_dir_path_0:
+        with temp_directory("bkup1") as temp_dir_path_1:
+            with temp_directory("bkup2") as temp_dir_path_2:
+                with temp_directory("bkup3") as temp_dir_path_3:
+                    with temp_directory("bkup4") as temp_dir_path_4:
+
+                        assert temp_dir_path_0.is_dir()
+                        assert temp_dir_path_1.is_dir()
+                        assert temp_dir_path_2.is_dir()
+                        assert temp_dir_path_3.is_dir()
+                        assert temp_dir_path_4.is_dir()
+
+                        # stop the salt_master, so can change user
+                        with salt_master.stopped():
+                            assert salt_master.is_running() is False
+
+                            for _path in log_pkg_paths:
+                                if bkup_count == 0:
+                                    cmd_to_run = (
+                                        f"cp -a {_path}/* {str(temp_dir_path_0)}/"
+                                    )
+                                elif bkup_count == 1:
+                                    cmd_to_run = (
+                                        f"cp -a {_path}/* {str(temp_dir_path_1)}/"
+                                    )
+                                elif bkup_count == 2:
+                                    cmd_to_run = (
+                                        f"cp -a {_path}/* {str(temp_dir_path_2)}/"
+                                    )
+                                elif bkup_count == 3:
+                                    cmd_to_run = (
+                                        f"cp -a {_path}/* {str(temp_dir_path_3)}/"
+                                    )
+                                elif bkup_count == 4:
+                                    cmd_to_run = (
+                                        f"cp -a {_path}/* {str(temp_dir_path_4)}/"
+                                    )
+                                elif bkup_count > 5:
+                                    assert bkupcount < bkup_count_max  # force assertion
+
+                                ret = salt_call_cli.run(
+                                    "--local", "cmd.run", cmd_to_run
+                                )
+                                bkup_count += 1
+                                assert ret.returncode == 0
+
+                            # change the user in the master's config file.
+                            ret = salt_call_cli.run(
+                                "--local",
+                                "file.replace",
+                                f"{install_salt.conf_dir}/master",
+                                "user: salt",
+                                f"user: {test_account.username}",
+                                "flags=['IGNORECASE']",
+                                "append_if_not_found=True",
+                            )
+                            assert ret.returncode == 0
+
+                            # change ownership of appropriate paths to user
+                            for _path in log_pkg_paths:
+                                chg_ownership_cmd = (
+                                    f"chown -R {test_account.username} {_path}"
+                                )
+                                ret = salt_call_cli.run(
+                                    "--local", "cmd.run", chg_ownership_cmd
+                                )
+                                assert ret.returncode == 0
+
+                            # restart the salt_master
+                            with salt_master.started():
+                                assert salt_master.is_running() is True
+
+                                # ensure some data in files
+                                log_files_list = [
+                                    "/var/log/salt/api",
+                                    "/var/log/salt/key",
+                                    "/var/log/salt/master",
+                                ]
+                                for _path in log_files_list:
+                                    log_path = pathlib.Path(_path)
+                                    assert log_path.exists()
+                                    with log_path.open("a") as f:
+                                        f.write("This is a log rotation test\n")
+
+                                # force log rotation
+                                logr_conf_file = "/etc/logrotate.d/salt"
+                                logr_conf_path = pathlib.Path(logr_conf_file)
+                                if not logr_conf_path.exists():
+                                    logr_conf_file = "/etc/logrotate.conf"
+                                    logr_conf_path = pathlib.Path(logr_conf_file)
+                                    assert logr_conf_path.exists()
+
+                                # force log rotation
+                                log_rotate_cmd = f"logrotate -f  {logr_conf_file}"
+                                ret = salt_call_cli.run(
+                                    "--local", "cmd.run", log_rotate_cmd
+                                )
+                                assert ret.returncode == 0
+
+                                for _path in log_files_list:
+                                    log_path = pathlib.Path(_path)
+                                    assert log_path.exists()
+                                    assert log_path.owner() == test_account.username
+                                    assert log_path.stat().st_mode & 0o7777 == 0o640
+
+                            # cleanup
+                            assert salt_master.is_running() is False
+
+                            # change the user in the master's config file.
+                            ret = salt_call_cli.run(
+                                "--local",
+                                "file.replace",
+                                f"{install_salt.conf_dir}/master",
+                                f"user: {test_account.username}",
+                                "user: salt",
+                                "flags=['IGNORECASE']",
+                                "append_if_not_found=True",
+                            )
+                            assert ret.returncode == 0
+
+                            # restore from backed up
+                            bkup_count = 0
+                            for _path in log_pkg_paths:
+                                if bkup_count == 0:
+                                    cmd_to_run = f"cp -a --force {str(temp_dir_path_0)}/* {_path}/"
+                                elif bkup_count == 1:
+                                    cmd_to_run = f"cp -a --force {str(temp_dir_path_1)}/* {_path}/"
+                                elif bkup_count == 2:
+                                    cmd_to_run = f"cp -a --force {str(temp_dir_path_2)}/* {_path}/"
+                                elif bkup_count == 3:
+                                    cmd_to_run = f"cp -a --force {str(temp_dir_path_3)}/* {_path}/"
+                                elif bkup_count == 4:
+                                    # use --update since /opt/saltstack/salt and would get SIGSEGV since mucking with running code
+                                    cmd_to_run = f"cp -a --update --force {str(temp_dir_path_4)}/* {_path}/"
+                                elif bkup_count > 5:
+                                    assert bkupcount < bkup_count_max  # force assertion
+
+                                ret = salt_call_cli.run(
+                                    "--local", "cmd.run", cmd_to_run
+                                )
+
+                                bkup_count += 1
+                                assert ret.returncode == 0