From 45ef5c174113c9133250dab82d629d7ab5cdc01b Mon Sep 17 00:00:00 2001
From: Dominique Martinet <asmadeus@codewreck.org>
Date: Wed, 2 Mar 2022 22:12:45 +0900
Subject: [PATCH] logrotate: add configuration check at build time
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Now the service no longer starts immediately,
check if the config we generated makes sense as soon as possible.

The check isn't perfect because logrotate --debug wants to check
users required, there are two problems:
 - /etc/passwd and /etc/group are sandboxed and we don't have
visibility of system users
 - the check phase runs as nixbld which cannot su to other users
and logrotate fails on this

Until these two problems can be addressed, users-related checks
are filtered out, it's still much better than no check.
The check can be disabled with services.logrotate.checkConfig
if required
(bird also has a preCheck param, to prepare the environment
before check, but we can add it if it becomes necessary)

Since this makes for very verbose builds, we only show errors:
There is no way to control log level, but logrotate hardcodes
'error:' at common log level, so we can use grep, taking care
to keep error codes

Some manual tests:
───────┬──────────────────────────────────────────
       │ File: valid-config.conf
───────┼──────────────────────────────────────────
   1   │ missingok
───────┴──────────────────────────────────────────
logrotate --debug ok
grep ok

───────┬──────────────────────────────────────────
       │ File: postrotate-no-end.conf
───────┼──────────────────────────────────────────
   1   │ missingok
   2   │ /file {
   3   │    postrotate
   4   │      test
   5   │ }
───────┴──────────────────────────────────────────
error: postrotate-no-end.conf:prerotate, postrotate or preremove without endscript

───────┬──────────────────────────────────────────
       │ File: missing-file.conf
───────┼──────────────────────────────────────────
   1   │ "test" { daily }
───────┴──────────────────────────────────────────
error: stat of test failed: No such file or directory

───────┬──────────────────────────────────────────
       │ File: unknown-option.conf
───────┼──────────────────────────────────────────
   1   │ some syntax error
───────┴──────────────────────────────────────────
logrotate --debug ok
error: unknown-option.conf:1 unknown option 'some' -- ignoring line

───────┬──────────────────────────────────────────
       │ File: unknown-user.conf
───────┼──────────────────────────────────────────
   1   │ su notauser notagroup
───────┴──────────────────────────────────────────
error: unknown-user.conf:1 unknown user 'notauser'

In particular note that logrotate would not error on unknown option
(it just ignores the line) but this change makes the check fail.
---
 nixos/modules/services/logging/logrotate.nix | 60 ++++++++++++++++++--
 nixos/tests/logrotate.nix                    |  8 +++
 2 files changed, 64 insertions(+), 4 deletions(-)

diff --git a/nixos/modules/services/logging/logrotate.nix b/nixos/modules/services/logging/logrotate.nix
index 6a9ed469fd38..c552d3b059c4 100644
--- a/nixos/modules/services/logging/logrotate.nix
+++ b/nixos/modules/services/logging/logrotate.nix
@@ -152,11 +152,45 @@ let
       { header = { global = true; priority = 100; }; }
     ]
   )));
-  configFile = pkgs.writeText "logrotate.conf" (
-    concatStringsSep "\n" (
+  configFile = pkgs.writeTextFile {
+    name = "logrotate.conf";
+    text = concatStringsSep "\n" (
       map mkConf settings
-    )
-  );
+    );
+    checkPhase = optionalString cfg.checkConfig ''
+      # logrotate --debug also checks that users specified in config
+      # file exist, but we only have sandboxed users here so brown these
+      # out. according to man page that means su, create and createolddir.
+      # files required to exist also won't be present, so missingok is forced.
+      user=$(${pkgs.coreutils}/bin/id -un)
+      group=$(${pkgs.coreutils}/bin/id -gn)
+      sed -e "s/\bsu\s.*/su $user $group/" \
+          -e "s/\b\(create\s\+[0-9]*\s*\|createolddir\s\+[0-9]*\s\+\).*/\1$user $group/" \
+          -e "1imissingok" -e "s/\bnomissingok\b//" \
+          $out > /tmp/logrotate.conf
+      # Since this makes for very verbose builds only show real error.
+      # There is no way to control log level, but logrotate hardcodes
+      # 'error:' at common log level, so we can use grep, taking care
+      # to keep error codes
+      set -o pipefail
+      if ! ${pkgs.logrotate}/sbin/logrotate --debug /tmp/logrotate.conf 2>&1 \
+          | ( ! grep "error:" ) > /tmp/logrotate-error; then
+              echo "Logrotate configuration check failed."
+              echo "The failing configuration (after adjustments to pass tests in sandbox) was:"
+              printf "%s\n" "-------"
+              cat /tmp/logrotate.conf
+              printf "%s\n" "-------"
+              echo "The error reported by logrotate was as follow:"
+              printf "%s\n" "-------"
+              cat /tmp/logrotate-error
+              printf "%s\n" "-------"
+              echo "You can disable this check with services.logrotate.checkConfig = false,"
+              echo "but if you think it should work please report this failure along with"
+              echo "the config file being tested!"
+              false
+      fi
+    '';
+  };
 
   mailOption =
     if foldr (n: a: a || n ? mail) false (attrValues cfg.settings)
@@ -256,6 +290,24 @@ in
         '';
       };
 
+      checkConfig = mkOption {
+        type = types.bool;
+        default = true;
+        description = ''
+          Whether the config should be checked at build time.
+
+          Some options are not checkable at build time because of the build sandbox:
+          for example, the test does not know about existing files and system users are
+          not known.
+          These limitations mean we must adjust the file for tests (missingok is forced
+          and users are replaced by dummy users).
+
+          Conversely there are still things that might make this check fail incorrectly
+          (e.g. a file path where we don't have access to intermediate directories):
+          in this case you can disable the failing check with this option.
+        '';
+      };
+
       # deprecated legacy compat settings
       paths = mkOption {
         type = with types; attrsOf (submodule pathOpts);
diff --git a/nixos/tests/logrotate.nix b/nixos/tests/logrotate.nix
index 3087119c339f..31592f0a39c5 100644
--- a/nixos/tests/logrotate.nix
+++ b/nixos/tests/logrotate.nix
@@ -40,6 +40,14 @@ import ./make-test-python.nix ({ pkgs, ... }: rec {
         postrotate = {
           postrotate = "touch /dev/null";
         };
+        # check checkConfig works as expected: there is nothing to check here
+        # except that the file build passes
+        checkConf = {
+          su = "root utmp";
+          createolddir = "0750 root utmp";
+          create = "root utmp";
+          "create " = "0750 root utmp";
+        };
         # multiple paths should be aggregated
         multipath = {
           files = [ "file1" "file2" ];