Fix logrotate.set dropping endscript on second script block (#68293)

changelog/68293.fixed.md

@@ -0,0 +1,5 @@
+Fixed ``logrotate.set`` dropping the second ``endscript`` (and turning
+embedded shell commands into bogus setting keys) when a stanza contained
+multiple script blocks such as both ``prerotate`` and ``postrotate``. Script
+directives are now parsed as opaque multi-line bodies and round-trip with
+their own ``endscript`` terminator each.

salt/modules/logrotate.py

@@ -13,6 +13,19 @@
 _LOG = logging.getLogger(__name__)
 _DEFAULT_CONF = "/etc/logrotate.conf"
 
+# Directives that introduce a multi-line shell script body terminated by
+# ``endscript``. The body must be kept opaque (not parsed as key/value
+# settings) so that lines like ``invoke-rc.d syslog-ng reload`` or repeated
+# ``endscript`` terminators do not collapse into bogus dict keys. See
+# the logrotate(8) manpage for the full list of script directives.
+_SCRIPT_DIRECTIVES = (
+    "firstaction",
+    "lastaction",
+    "prerotate",
+    "postrotate",
+    "preremove",
+)
+
 
 # Define a function alias in order not to shadow built-in's
 __func_alias__ = {"set_": "set"}
@@ -61,10 +74,31 @@ def _parse_conf(conf_file=_DEFAULT_CONF):
     multi_names = []
     multi = {}
     prev_comps = None
+    # When inside a ``prerotate``/``postrotate``/... block, collect the raw
+    # script body lines here and stash them on the enclosing dict under the
+    # script directive name once ``endscript`` is seen.
+    script_directive = None
+    script_body = []
 
     with salt.utils.files.fopen(conf_file, "r") as ifile:
         for line in ifile:
-            line = salt.utils.stringutils.to_unicode(line).strip()
+            line = salt.utils.stringutils.to_unicode(line).rstrip("\r\n")
+            stripped = line.strip()
+
+            if script_directive is not None:
+                # Inside a script block: lines are opaque shell content until
+                # the ``endscript`` terminator. Do not strip comments or skip
+                # blank lines — they are part of the script.
+                if stripped == "endscript":
+                    target = multi if mode == "multi" else ret
+                    target[script_directive] = list(script_body)
+                    script_directive = None
+                    script_body = []
+                    continue
+                script_body.append(stripped)
+                continue
+
+            line = stripped
             if not line:
                 continue
             if line.startswith("#"):
@@ -106,6 +140,15 @@ def _parse_conf(conf_file=_DEFAULT_CONF):
                         ret[file_key] = include_conf[file_key]
                         ret["include files"][include].append(file_key)
 
+            # Recognize the start of a script block. The body that follows
+            # (up to ``endscript``) is kept opaque so embedded shell commands
+            # don't get parsed as settings and so a second script block's
+            # ``endscript`` is not lost to a duplicate-key collision.
+            if comps[0] in _SCRIPT_DIRECTIVES and len(comps) == 1:
+                script_directive = comps[0]
+                script_body = []
+                continue
+
             prev_comps = comps
             if len(comps) > 2:
                 key[comps[0]] = " ".join(comps[1:])
@@ -277,7 +320,20 @@ def _dict_to_stanza(key, stanza):
     """
     ret = ""
     for skey in stanza:
-        if stanza[skey] is True:
+        value = stanza[skey]
+        # Script directives (``prerotate``/``postrotate``/...) are stored as
+        # a list of opaque body lines; emit them as a script block followed
+        # by an ``endscript`` terminator. Without this, a stanza containing
+        # both ``prerotate`` and ``postrotate`` would lose the second
+        # ``endscript`` on round-trip — see issue #68293.
+        if skey in _SCRIPT_DIRECTIVES and isinstance(value, list):
+            ret += f"    {skey}\n"
+            for body_line in value:
+                ret += f"        {body_line}\n"
+            ret += "    endscript\n"
+            continue
+        if value is True:
+            value = ""
             stanza[skey] = ""
-        ret += f"    {skey} {stanza[skey]}\n"
+        ret += f"    {skey} {value}\n"
     return f"{key} {{\n{ret}}}"

tests/pytests/unit/modules/test_logrotate.py

@@ -4,6 +4,8 @@
     Test cases for salt.modules.logrotate
 """
 
+import textwrap
+
 import pytest
 
 import salt.modules.logrotate as logrotate
@@ -81,6 +83,74 @@ def test_set_setting_failed(PARSE_CONF):
         pytest.raises(SaltInvocationError, logrotate.set_, **kwargs)
 
 
+def test_parse_conf_preserves_script_blocks(tmp_path):
+    """
+    Regression test for #68293.
+
+    ``_parse_conf`` must treat the body of ``prerotate``/``postrotate``/
+    ``firstaction``/``lastaction``/``preremove`` blocks as opaque script
+    content terminated by ``endscript``. Previously every line inside the
+    block was treated as a key/value setting, so the two ``endscript`` lines
+    collapsed into a single dict key, the embedded shell commands became
+    bogus setting keys, and rewriting the stanza dropped the second
+    ``endscript`` — breaking the rendered logrotate configuration.
+    """
+    conf = textwrap.dedent(
+        """\
+        /parsec/log/system/events {
+            rotate 10
+            size 18M
+            missingok
+            notifempty
+            compress
+            delaycompress
+            sharedscripts
+            prerotate
+            chattr -a /parsec/log/system/events > /dev/null
+            endscript
+            postrotate
+            system-protect-event-log > /dev/null
+            invoke-rc.d syslog-ng reload > /dev/null
+            endscript
+        }
+        """
+    )
+    conf_file = tmp_path / "logrotate.conf"
+    conf_file.write_text(conf)
+
+    parsed = logrotate._parse_conf(str(conf_file))
+    stanza = parsed["/parsec/log/system/events"]
+
+    # The script bodies are preserved verbatim, keyed by the script directive.
+    assert stanza["prerotate"] == [
+        "chattr -a /parsec/log/system/events > /dev/null",
+    ]
+    assert stanza["postrotate"] == [
+        "system-protect-event-log > /dev/null",
+        "invoke-rc.d syslog-ng reload > /dev/null",
+    ]
+
+    # The shell commands inside the script blocks must not leak into the
+    # stanza as bogus setting keys.
+    for leaked in ("chattr", "system-protect-event-log", "invoke-rc.d"):
+        assert leaked not in stanza, (
+            f"{leaked!r} was parsed as a setting key — script block was not"
+            " kept opaque"
+        )
+
+    # ``endscript`` is a block terminator, not a setting in its own right.
+    assert "endscript" not in stanza
+
+    # ``_dict_to_stanza`` must round-trip both script blocks with their own
+    # ``endscript`` terminator each.
+    rendered = logrotate._dict_to_stanza("/parsec/log/system/events", stanza)
+    assert rendered.count("endscript") == 2, rendered
+    assert "prerotate\n" in rendered, rendered
+    assert "postrotate\n" in rendered, rendered
+    assert "chattr -a /parsec/log/system/events > /dev/null" in rendered
+    assert "invoke-rc.d syslog-ng reload > /dev/null" in rendered
+
+
 def test_get(PARSE_CONF):
     """
     Test if get a value for a specific configuration line