Added --simulate-keep-chroot option to keep the changes chroot after a run

cf-agent/cf-agent.c

@@ -221,6 +221,7 @@ static const struct option OPTIONS[] =
     {"skip-bootstrap-service-start", no_argument, 0, 0 },
     {"skip-db-check", optional_argument, 0, 0 },
     {"simulate", required_argument, 0, 0},
+    {"simulate-keep-chroot", required_argument, 0, 0},
     {NULL, 0, 0, '\0'}
 };
 
@@ -257,6 +258,7 @@ static const char *const HINTS[] =
     "Do not start CFEngine services as part of the bootstrap process",
     "Do not run database integrity checks and repairs at startup",
     "Run in simulate mode, either 'manifest', 'manifest-full' or 'diff'",
+    "Keep the changes chroot from a --simulate run, creating it at the given path, which must not already exist",
     NULL
 };
 
@@ -795,6 +797,38 @@ static GenericAgentConfig *CheckOpts(int argc, char **argv)
                     DoCleanupAndExit(EXIT_FAILURE);
                 }
             }
+            else if (StringEqual(option_name, "simulate-keep-chroot"))
+            {
+                if (optarg == NULL)
+                {
+                    Log(LOG_LEVEL_ERR,
+                        "Missing argument for --simulate-keep-chroot, a directory path required");
+                    DoCleanupAndExit(EXIT_FAILURE);
+                }
+                else if (!IsAbsPath(optarg))
+                {
+                    Log(LOG_LEVEL_ERR,
+                        "Invalid argument for --simulate-keep-chroot, an absolute path required, not '%s'",
+                        optarg);
+                    DoCleanupAndExit(EXIT_FAILURE);
+                }
+                else if (strlen(optarg) > (PATH_MAX / 2))
+                {
+                    /* Every path the run touches is mapped to a path under
+                     * this directory and the result has to fit in PATH_MAX,
+                     * so roughly half of that budget is reserved for the
+                     * original paths (and mapping a path that still does not
+                     * fit aborts the run instead of truncating the path). */
+                    Log(LOG_LEVEL_ERR,
+                        "Invalid argument for --simulate-keep-chroot, path longer than %d bytes",
+                        PATH_MAX / 2);
+                    DoCleanupAndExit(EXIT_FAILURE);
+                }
+                else
+                {
+                    config->agent_specific.agent.simulate_keep_chroot = xstrdup(optarg);
+                }
+            }
             break;
         }
         default:
@@ -822,6 +856,14 @@ static GenericAgentConfig *CheckOpts(int argc, char **argv)
         DoCleanupAndExit(EXIT_FAILURE);
     }
 
+    if ((config->agent_specific.agent.simulate_keep_chroot != NULL) &&
+        !ChrootChanges())
+    {
+        Log(LOG_LEVEL_ERR,
+            "Option --simulate-keep-chroot can only be used together with --simulate");
+        DoCleanupAndExit(EXIT_FAILURE);
+    }
+
     FreeFixedStringArray(argc_new, argv_new);
 
     return config;

libpromises/eval_context.c

@@ -3872,17 +3872,18 @@ const char *ToChangesChroot(const char *orig_path)
 
     assert(orig_path != NULL);
     assert(IsAbsPath(orig_path));
-    assert(strlen(orig_path) <= (PATH_MAX - chroot_len - 1));
+
+    const char *const given_path = orig_path;
 
     size_t offset = 0;
 #ifdef __MINGW32__
     /* On Windows, absolute path starts with the drive letter and colon followed
      * by '\'. Let's replace the ":\" with just "\" so that each drive has its
      * own directory tree in the chroot. */
+    char drive_letter = '\0';
     if ((orig_path[0] > 'A') && ((orig_path[0] < 'Z')) && (orig_path[1] == ':'))
     {
-        chrooted_path[chroot_len] = orig_path[0];
-        chrooted_path[chroot_len + 1] = FILE_SEPARATOR;
+        drive_letter = orig_path[0];
         orig_path += 2;
         offset += 2;
     }
@@ -3893,8 +3894,29 @@ const char *ToChangesChroot(const char *orig_path)
         orig_path++;
     }
 
-    /* Adds/copies the NUL-byte at the end of the string. */
-    strncpy(chrooted_path + chroot_len + offset, orig_path, (PATH_MAX - chroot_len - offset - 1));
+    /* A path that does not fit must not be truncated -- the copy would
+     * silently be made at a different path than the one recorded and
+     * reported, and two long paths could even be mapped to the same copy.
+     * Checked before anything is written into the buffer. */
+    const size_t orig_len = strlen(orig_path);
+    if ((chroot_len + offset + orig_len) >= PATH_MAX)
+    {
+        Log(LOG_LEVEL_ERR,
+            "The path '%s' is too long to be mapped into the changes chroot, aborting",
+            given_path);
+        DoCleanupAndExit(EXIT_FAILURE);
+    }
+
+#ifdef __MINGW32__
+    if (drive_letter != '\0')
+    {
+        chrooted_path[chroot_len] = drive_letter;
+        chrooted_path[chroot_len + 1] = FILE_SEPARATOR;
+    }
+#endif
+
+    /* Copies the NUL-byte at the end of the string. */
+    memcpy(chrooted_path + chroot_len + offset, orig_path, orig_len + 1);
 
     return chrooted_path;
 }

libpromises/generic_agent.c

@@ -89,6 +89,10 @@ static char PIDFILE[CF_BUFSIZE] = ""; /* GLOBAL_C */
 /* Used for 'ident' argument to openlog() */
 static char CF_PROGRAM_NAME[256] = "";
 
+/* Path of the changes chroot kept after a run because of
+ * --simulate-keep-chroot, reported at exit by KeepChangesChroot(). */
+static char KEEP_CHANGES_CHROOT[PATH_MAX] = ""; /* GLOBAL_C */
+
 static void CheckWorkingDirectories(EvalContext *ctx);
 
 static void GetAutotagDir(char *dirname, size_t max_size, const char *maybe_dirname);
@@ -106,6 +110,7 @@ static bool LoadAugmentsFiles(EvalContext *ctx, const char* filename);
 
 static void GetChangesChrootDir(char *buf, size_t buf_size);
 static void DeleteChangesChroot();
+static void KeepChangesChroot();
 static int ParseFacility(const char *name);
 static inline const char *LogFacilityToString(int facility);
 
@@ -1629,9 +1634,89 @@ void GenericAgentInitialize(EvalContext *ctx, GenericAgentConfig *config)
     if (ChrootChanges())
     {
         char changes_chroot[PATH_MAX] = {0};
-        GetChangesChrootDir(changes_chroot, sizeof(changes_chroot));
+        const char *keep_chroot =
+            config->agent_specific.agent.simulate_keep_chroot;
+        if (keep_chroot != NULL)
+        {
+            strlcpy(changes_chroot, keep_chroot, sizeof(changes_chroot));
+
+            /* Strip any trailing separators so that the parent directory is
+             * determined correctly below. */
+            DeleteSlash(changes_chroot);
+
+            /* The chroot is created in the requested directory and kept after
+             * the run instead of being deleted. The directory is required to
+             * not exist yet so that the copies of potentially sensitive system
+             * files made in it below cannot mix with stale contents from a
+             * previous run and are only made in a directory created by this
+             * process, with the permissions enforced below. */
+#ifndef __MINGW32__
+            /* Creating the directory with restrictive permissions is no
+             * protection if another user can replace the directory itself, so
+             * refuse a parent directory writable by group or others, unless
+             * the sticky bit keeps them from renaming or unlinking entries
+             * they don't own. */
+            char *parent = GetParentDirectoryCopy(changes_chroot);
+            if (parent == NULL)
+            {
+                FatalError(
+                    ctx,
+                    "Failed to determine the parent directory of '%s' for keeping the changes chroot",
+                    changes_chroot);
+            }
+            struct stat sb;
+            if (stat(parent, &sb) != 0)
+            {
+                FatalError(
+                    ctx,
+                    "Failed to check the parent directory '%s' for keeping the changes chroot (stat: %s)",
+                    parent,
+                    GetErrorStr());
+            }
+            if (((sb.st_mode & (S_IWGRP | S_IWOTH)) != 0) &&
+                ((sb.st_mode & S_ISVTX) == 0))
+            {
+                FatalError(
+                    ctx,
+                    "Refusing to create the directory '%s' for keeping the changes chroot, its parent directory '%s' is writable by other users",
+                    changes_chroot,
+                    parent);
+            }
+            free(parent);
+#endif /* !__MINGW32__ */
+
+            if (mkdir(changes_chroot, 0700) != 0)
+            {
+                FatalError(
+                    ctx,
+                    "Failed to create the directory '%s' for keeping the changes chroot (mkdir: %s)",
+                    changes_chroot,
+                    GetErrorStr());
+            }
+
+            /* mkdir()'s mode argument is masked by umask, make sure the
+             * directory really is only accessible to its owner. */
+            if (chmod(changes_chroot, 0700) != 0)
+            {
+                FatalError(
+                    ctx,
+                    "Failed to set the permissions of the directory '%s' for keeping the changes chroot (chmod: %s)",
+                    changes_chroot,
+                    GetErrorStr());
+            }
+
+            strlcpy(
+                KEEP_CHANGES_CHROOT,
+                changes_chroot,
+                sizeof(KEEP_CHANGES_CHROOT));
+            RegisterCleanupFunction(KeepChangesChroot);
+        }
+        else
+        {
+            GetChangesChrootDir(changes_chroot, sizeof(changes_chroot));
+            RegisterCleanupFunction(DeleteChangesChroot);
+        }
         SetChangesChroot(changes_chroot);
-        RegisterCleanupFunction(DeleteChangesChroot);
         Log(LOG_LEVEL_WARNING, "All changes in files will be made in the '%s' chroot",
             changes_chroot);
     }
@@ -1799,6 +1884,11 @@ static void DeleteChangesChroot()
     }
 }
 
+static void KeepChangesChroot()
+{
+    Log(LOG_LEVEL_NOTICE, "Keeping changes chroot '%s'", KEEP_CHANGES_CHROOT);
+}
+
 void GenericAgentFinalize(EvalContext *ctx, GenericAgentConfig *config)
 {
     /* TODO, FIXME: what else from the above do we need to undo here ? */
@@ -2614,6 +2704,9 @@ GenericAgentConfig *GenericAgentConfigNewDefault(AgentType agent_type, bool tty_
     /* By default we start services during bootstrap */
     config->agent_specific.agent.skip_bootstrap_service_start = false;
 
+    /* By default the changes chroot from a simulate run is deleted at exit */
+    config->agent_specific.agent.simulate_keep_chroot = NULL;
+
     /* Log classes */
     config->agent_specific.agent.report_class_log = false;
 
@@ -2661,6 +2754,7 @@ void GenericAgentConfigDestroy(GenericAgentConfig *config)
         free(config->agent_specific.agent.bootstrap_host);
         free(config->agent_specific.agent.bootstrap_ip);
         free(config->agent_specific.agent.bootstrap_port);
+        free(config->agent_specific.agent.simulate_keep_chroot);
         free(config);
     }
 }

libpromises/generic_agent.h

@@ -94,6 +94,7 @@ typedef struct
             bool skip_bootstrap_service_start;
             char *show_evaluated_classes;
             char *show_evaluated_variables;
+            char *simulate_keep_chroot; // --simulate-keep-chroot
 
             // BODY AGENT CONTROL
             bool report_class_log;

tests/acceptance/29_simulate_mode/keep_chroot.cf

@@ -0,0 +1,86 @@
+body common control
+{
+  inputs => {
+    "../default.sub.cf",
+  };
+  bundlesequence => { default("$(this.promise_filename)") };
+  version => "1.0";
+}
+
+bundle agent init
+{
+  files:
+    "$(G.testdir)$(const.dirsep)keep-chroot-file"
+      create => "true",
+      content => "This is the original content of the file.";
+}
+
+bundle agent test
+{
+  meta:
+    "test_soft_fail"
+      string => "(solaris|aix|hpux|windows)",
+      # ENT-6540 exotics fail to delete chroot
+      # On Windows the chroot maps drive letters to a different layout so the
+      # path of the kept copy checked below does not apply there
+      meta => { "ENT-6540" };
+
+    "description"
+      string => "Test that --simulate-keep-chroot keeps the changes chroot and that it is still deleted by default";
+
+  vars:
+    "kept_chroot" string => "$(G.testdir)$(const.dirsep)kept.changes";
+
+  commands:
+    # add --verbose here and look at the log for debugging sub policy runs
+    "$(sys.cf_agent) -Kf $(this.promise_dirname)$(const.dirsep)keep_chroot.cf.sub --simulate=manifest --simulate-keep-chroot=$(kept_chroot) > $(G.testdir)$(const.dirsep)keep.log 2>&1"
+      contain => in_shell,
+      comment => "Run sub policy in simulate mode, keeping the changes chroot";
+
+    "$(sys.cf_agent) -Kf $(this.promise_dirname)$(const.dirsep)keep_chroot.cf.sub --simulate=manifest > $(G.testdir)$(const.dirsep)default.log 2>&1"
+      contain => in_shell,
+      comment => "Run sub policy in simulate mode with the default chroot cleanup";
+}
+
+bundle agent check
+{
+  vars:
+    "kept_copy"
+      string => "$(test.kept_chroot)$(G.testdir)$(const.dirsep)keep-chroot-file";
+
+  classes:
+    "kept_copy_has_changes"
+      expression => strcmp(readfile("$(kept_copy)", 0),
+                           "This is the would-be content of the file."),
+      if => fileexists("$(kept_copy)");
+
+    "real_file_untouched"
+      expression => strcmp(readfile("$(G.testdir)$(const.dirsep)keep-chroot-file", 0),
+                           "This is the original content of the file."),
+      if => fileexists("$(G.testdir)$(const.dirsep)keep-chroot-file");
+
+    "keep_notice_logged"
+      expression => regline(".*Keeping changes chroot.*",
+                            "$(G.testdir)$(const.dirsep)keep.log"),
+      if => fileexists("$(G.testdir)$(const.dirsep)keep.log");
+
+    # The default.log check below makes sure this is not evaluated before the
+    # run without --simulate-keep-chroot has actually happened
+    "default_chroot_deleted"
+      expression => strcmp(length(findfiles("$(sys.statedir)$(const.dirsep)*.changes")), "0"),
+      if => fileexists("$(G.testdir)$(const.dirsep)default.log");
+
+    "keep_requires_simulate"
+      expression => not(returnszero("$(sys.cf_agent) -Kf $(this.promise_dirname)$(const.dirsep)keep_chroot.cf.sub --simulate-keep-chroot=$(G.testdir)$(const.dirsep)unused.changes", "noshell")),
+      if => fileexists("$(G.testdir)$(const.dirsep)default.log");
+
+    "ok"
+      expression => and("kept_copy_has_changes", "real_file_untouched",
+                        "keep_notice_logged", "default_chroot_deleted",
+                        "keep_requires_simulate");
+
+  methods:
+    "Pass/Fail"
+      usebundle => dcs_passif("ok", "$(this.promise_filename)"),
+      inherit => "true"; # We want dcs_passif to inherit bundle scoped classes from our check bundle
+}

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

@@ -0,0 +1,17 @@
+bundle common simulate_keep_chroot_promises
+{
+  vars:
+    "inputs" slist => { "$(this.promise_dirname)/../default.sub.cf" };
+}
+
+body common control
+{
+  inputs => { "@(simulate_keep_chroot_promises.inputs)" };
+}
+
+bundle agent main
+{
+  files:
+    "$(G.testdir)$(const.dirsep)keep-chroot-file"
+      content => "This is the would-be content of the file.";
+}