Fixed --simulate=diff and --simulate symlink reporting (3.24)

cf-agent/simulate_mode.c

@@ -46,8 +46,13 @@
 
 #define DELIM_CHAR '='
 
-/* Taken from coreutils/lib/stat-macros.h. */
+/* Taken from coreutils/lib/stat-macros.h. Windows has no setuid, setgid or
+ * sticky bits. */
+#ifndef __MINGW32__
 #define CHMOD_MODE_BITS (S_ISUID | S_ISGID | S_ISVTX | S_IRWXU | S_IRWXG | S_IRWXO)
+#else
+#define CHMOD_MODE_BITS (S_IRWXU | S_IRWXG | S_IRWXO)
+#endif
 
 static inline void PrintDelimiter()
 {
@@ -326,7 +331,9 @@ static inline void ManifestFileDetails(const char *path, struct stat *st, bool c
     }
 }
 
-bool ManifestFile(const char *path, bool chrooted)
+/* st_orig, if not NULL, is the original file, to report a change of type. */
+static bool ManifestFileInternal(const char *path, bool chrooted,
+                                 const struct stat *st_orig)
 {
     PrintDelimiter();
     const char *real_path = path;
@@ -343,7 +350,17 @@ bool ManifestFile(const char *path, bool chrooted)
         return true;
     }
 
-    printf("'%s' is a %s\n", path, GetFileTypeDescription(st.st_mode));
+    if ((st_orig != NULL) &&
+        ((st_orig->st_mode & S_IFMT) != (st.st_mode & S_IFMT)))
+    {
+        printf("'%s' changed type from %s to %s\n", path,
+               GetFileTypeDescription(st_orig->st_mode),
+               GetFileTypeDescription(st.st_mode));
+    }
+    else
+    {
+        printf("'%s' is a %s\n", path, GetFileTypeDescription(st.st_mode));
+    }
     ManifestStatInfo(&st);
     ManifestFileDetails(real_path, &st, chrooted);
 
@@ -357,6 +374,11 @@ bool ManifestRename(const char *orig_name, const char *new_name)
     return true;
 }
 
+bool ManifestFile(const char *path, bool chrooted)
+{
+    return ManifestFileInternal(path, chrooted, NULL);
+}
+
 static bool RunDiff(const char *path1, const char *path2)
 {
     char diff_path[PATH_MAX];
@@ -418,12 +440,74 @@ static bool RunDiff(const char *path1, const char *path2)
     return !failure;
 }
 
-static bool DiffFile(const char *path)
+#ifndef __MINGW32__
+static inline void FormatUser(uid_t uid, char *buf, size_t buf_size)
 {
-    const char *chrooted_path = ToChangesChroot(path);
+    char name[CF_SMALLBUF];
+    NDEBUG_UNUSED int ret;
+    if (GetUserName(uid, name, sizeof(name), LOG_LEVEL_VERBOSE))
+    {
+        ret = snprintf(buf, buf_size, "%ju/%s", (uintmax_t) uid, name);
+    }
+    else
+    {
+        ret = snprintf(buf, buf_size, "%ju", (uintmax_t) uid);
+    }
+    /* Can't truncate: callers pass CF_BUFSIZE, name is at most CF_SMALLBUF. */
+    assert((ret >= 0) && ((size_t) ret < buf_size));
+}
+
+static inline void FormatGroup(gid_t gid, char *buf, size_t buf_size)
+{
+    char name[CF_SMALLBUF];
+    NDEBUG_UNUSED int ret;
+    if (GetGroupName(gid, name, sizeof(name), LOG_LEVEL_VERBOSE))
+    {
+        ret = snprintf(buf, buf_size, "%ju/%s", (uintmax_t) gid, name);
+    }
+    else
+    {
+        ret = snprintf(buf, buf_size, "%ju", (uintmax_t) gid);
+    }
+    /* Can't truncate: callers pass CF_BUFSIZE, name is at most CF_SMALLBUF. */
+    assert((ret >= 0) && ((size_t) ret < buf_size));
+}
+#endif  /* !__MINGW32__ */
+
+/* Report permission and ownership changes too, 'diff' only compares contents.
+ * Owner and group are only compared on POSIX. */
+static void DiffStatInfo(const char *path, const struct stat *st_orig, const struct stat *st_chrooted)
+{
+    assert(st_orig != NULL);
+    assert(st_chrooted != NULL);
+
+    if ((st_orig->st_mode & CHMOD_MODE_BITS) != (st_chrooted->st_mode & CHMOD_MODE_BITS))
+    {
+        printf("'%s' changed permissions from %04o to %04o\n", path,
+               st_orig->st_mode & CHMOD_MODE_BITS, st_chrooted->st_mode & CHMOD_MODE_BITS);
+    }
+
+#ifndef __MINGW32__
+    char from[CF_BUFSIZE];
+    char to[CF_BUFSIZE];
+    if (st_orig->st_uid != st_chrooted->st_uid)
+    {
+        FormatUser(st_orig->st_uid, from, sizeof(from));
+        FormatUser(st_chrooted->st_uid, to, sizeof(to));
+        printf("'%s' changed owner from %s to %s\n", path, from, to);
+    }
+    if (st_orig->st_gid != st_chrooted->st_gid)
+    {
+        FormatGroup(st_orig->st_gid, from, sizeof(from));
+        FormatGroup(st_chrooted->st_gid, to, sizeof(to));
+        printf("'%s' changed group from %s to %s\n", path, from, to);
+    }
+#endif  /* !__MINGW32__ */
+}
 
+static bool DiffFile(const char *path)
+{
     struct stat st_orig;
-    struct stat st_chrooted;
     if (lstat(path, &st_orig) == -1)
     {
         /* Original final doesn't exist, must be a new file in the changes
@@ -432,37 +516,19 @@ static bool DiffFile(const char *path)
         return true;
     }
 
-    PrintDelimiter();
-    if (lstat(chrooted_path, &st_chrooted) == -1)
+    const char *chrooted_path = ToChangesChroot(path);
+    struct stat st_chrooted;
+    if ((lstat(chrooted_path, &st_chrooted) == 0) &&
+        ((st_orig.st_mode & S_IFMT) == (st_chrooted.st_mode & S_IFMT)) &&
+        (S_ISREG(st_chrooted.st_mode) || S_ISDIR(st_chrooted.st_mode)))
     {
-        /* TODO: should this do print info about the original file? */
-        printf("'%s' no longer exists\n", path);
-        return true;
+        PrintDelimiter();
+        DiffStatInfo(path, &st_orig, &st_chrooted);
+        return RunDiff(path, chrooted_path);
     }
 
-    if ((st_orig.st_mode & S_IFMT) != (st_chrooted.st_mode & S_IFMT))
-    {
-        /* File type changed. */
-        printf("'%s' changed type from %s to %s\n", path,
-               GetFileTypeDescription(st_orig.st_mode),
-               GetFileTypeDescription(st_chrooted.st_mode));
-        ManifestStatInfo(&st_chrooted);
-        ManifestFileDetails(path, &st_chrooted, true);
-        return true;
-    }
-    else
-    {
-        switch (st_chrooted.st_mode & S_IFMT) {
-        case S_IFREG:
-        case S_IFDIR:
-            return RunDiff(path, chrooted_path);
-        default:
-            printf("'%s' is a %s\n", path, GetFileTypeDescription(st_chrooted.st_mode));
-            ManifestStatInfo(&st_chrooted);
-            ManifestFileDetails(path, &st_chrooted, true);
-            return true;
-        }
-    }
+    /* Gone, changed type, or not something 'diff' can compare. */
+    return ManifestFileInternal(path, true, &st_orig);
 }
 
 bool ManifestRenamedFiles()

libpromises/files_links.c

@@ -180,14 +180,22 @@ PromiseResult VerifyLink(EvalContext *ctx, char *destination, const char *source
             return PROMISE_RESULT_NOOP;
         }
         /* else */
+
+        /* The mirrored symlink's target is in the chroot, show the real one. */
+        const char *real_linkbuf = linkbuf;
+        if (ChrootChanges() && IsAbsoluteFileName(linkbuf))
+        {
+            real_linkbuf = ToNormalRoot(linkbuf);
+        }
+
         if (attr->move_obstructions)
         {
             if (MakingChanges(ctx, pp, attr, &result, "remove incorrect link '%s'", destination))
             {
                 if (unlink(ToChangesPath(destination)) == -1)
                 {
                     RecordFailure(ctx, pp, attr, "Error removing link '%s' (points to '%s' not '%s')",
-                                  destination, linkbuf, to);
+                                  destination, real_linkbuf, to);
                     return PROMISE_RESULT_FAIL;
                 }
                 RecordChange(ctx, pp, attr, "Overrode incorrect link '%s'", destination);
@@ -205,7 +213,7 @@ PromiseResult VerifyLink(EvalContext *ctx, char *destination, const char *source
         {
             RecordFailure(ctx, pp, attr,
                           "Link '%s' points to '%s' not '%s', but not moving obstructions",
-                          destination, linkbuf, to);
+                          destination, real_linkbuf, to);
             return PROMISE_RESULT_FAIL;
         }
     }

tests/acceptance/29_simulate_mode/diff_mode.cf.expected

@@ -102,6 +102,7 @@ Only in WORKDIR/tmp/sub-dir/.: sub-file
 -<Root/>
 +<Root><x foo="bar">y</x></Root>
 ===========================================================================
+'WORKDIR/tmp/perms' changed permissions from 0600 to 0000
 ===========================================================================
 'WORKDIR/tmp/transformer' no longer exists
 ===========================================================================

tests/acceptance/29_simulate_mode/diff_mode_ownership.cf

@@ -0,0 +1,63 @@
+body common control
+{
+  inputs => { "../default.cf.sub", "./normalize_agent_output.cf.sub" };
+  bundlesequence => { default("$(this.promise_filename)") };
+  version => "1.0";
+}
+
+bundle agent init
+{
+  meta:
+    "test_skip_needs_work"
+      string => "using_fakeroot",
+      comment => "chown is faked under fakeroot; the chrooted stat won't reflect real ownership changes";
+
+  files:
+    "$(G.testroot)$(const.dirsep)chown-me"
+      create => "true",
+      comment => "Created as root:root, will be promised to bin:bin in the sub policy";
+}
+
+bundle agent test
+{
+  meta:
+    "test_soft_fail"
+      string => "(solaris|aix|hpux|windows)",
+      meta => { "ENT-6540", "ENT-10254" };
+
+    "description" -> { "ENT-14694" }
+      string => "Test that --simulate=diff reports owner and group changes";
+
+  commands:
+    "$(sys.cf_agent) -Kf $(this.promise_dirname)$(const.dirsep)diff_mode_ownership.cf.sub --simulate=diff 2>&1 | $(G.grep) 'changed owner\|changed group' > $(this.promise_filename).actual"
+      contain => in_shell;
+}
+
+bundle agent check
+{
+  vars:
+    "actual" string => readfile("$(this.promise_filename).actual", inf);
+
+  classes:
+    "has_owner_change"
+      expression => regcmp(
+        ".*changed owner from 0/root to [0-9]+/bin.*", "$(actual)"
+      );
+
+    "has_group_change"
+      expression => regcmp(
+        ".*changed group from 0/root to [0-9]+/bin.*", "$(actual)"
+      );
+
+    "ok" and => { "has_owner_change", "has_group_change" };
+
+  reports:
+    ok::
+      "$(this.promise_filename) Pass";
+
+    !ok::
+      "$(this.promise_filename) FAIL";
+
+    !ok::
+      "actual output: $(actual)";
+}

tests/acceptance/29_simulate_mode/diff_mode_ownership.cf.sub

@@ -0,0 +1,31 @@
+bundle common simulation_ownership_promises
+{
+  vars:
+    "inputs" slist => { "$(this.promise_dirname)/../default.cf.sub" };
+}
+
+body common control
+{
+  inputs => { "@(simulation_ownership_promises.inputs)" };
+}
+
+bundle agent init
+{
+  files:
+    "$(G.testroot)$(const.dirsep)chown-me"
+      create => "true";
+}
+
+bundle agent test
+{
+  files:
+    "$(G.testroot)$(const.dirsep)chown-me"
+      perms => mog("0600", "bin", "bin");
+}
+
+bundle agent main
+{
+  methods:
+    "init";
+    "test";
+}

tests/acceptance/29_simulate_mode/diff_mode_symlinks.cf

@@ -0,0 +1,60 @@
+body common control
+{
+  inputs => { "../default.cf.sub" };
+  bundlesequence => { default("$(this.promise_filename)") };
+  version => "1.0";
+}
+
+bundle agent init
+{
+  files:
+    "$(G.testroot)/target-old" create => "true";
+    "$(G.testroot)/target-new" create => "true";
+
+    "$(G.testroot)/wrong-target"
+      link_from => linkfrom("$(G.testroot)/target-old", "symlink");
+
+    "$(G.testroot)/obstruction"
+      create => "true",
+      content => "I am in the way";
+}
+
+bundle agent test
+{
+  meta:
+    "test_soft_fail"
+      string => "(solaris|aix|hpux|windows)",
+      # ENT-6540 exotics fail to delete chroot
+      # ENT-10254 tests fail on Windows due to CRLF
+      meta => { "ENT-6540", "ENT-10254" };
+
+    "description" -> { "ENT-14713" }
+      string => "Test that --simulate=diff shows the new target of a symlink that replaced a wrong link or an obstructing file";
+}
+
+bundle agent check
+{
+  vars:
+    "command"
+      string => "$(sys.cf_agent) -Kf $(this.promise_filename).sub --simulate=diff";
+
+    "new_target" string => "\QTarget: '$(G.testroot)/target-new'\E";
+
+    # Bound the gap to one file's section (6 output lines today, +1 slack)
+    "same_section" string => "\n(?:[^\n]*\n){0,7}";
+
+    "wanted"
+      string => concat(
+        ".*\Q'$(G.testroot)/wrong-target' is a symbolic link\E$(same_section)$(new_target)",
+        ".*\Q'$(G.testroot)/obstruction' changed type from regular file to symbolic link\E$(same_section)$(new_target).*"
+      );
+
+  methods:
+    "check"
+      usebundle => dcs_passif_output(
+        $(wanted),
+        ".*(Invalid target|Target: '[^/]).*",
+        $(command),
+        $(this.promise_filename)
+      );
+}

tests/acceptance/29_simulate_mode/diff_mode_symlinks.cf.sub

@@ -0,0 +1,15 @@
+body common control
+{
+  inputs => { "../default.cf.sub" };
+}
+
+bundle agent main
+{
+  files:
+    "$(G.testroot)/wrong-target"
+      link_from => linkfrom("$(G.testroot)/target-new", "symlink"),
+      move_obstructions => "true";
+    "$(G.testroot)/obstruction"
+      link_from => linkfrom("$(G.testroot)/target-new", "symlink"),
+      move_obstructions => "true";
+}