From 3746a4294d13781cdbd4c9646d7b1d20a88c872c Mon Sep 17 00:00:00 2001 From: Nick Anderson Date: Fri, 23 Oct 2026 18:35:47 -0500 Subject: [PATCH 1/3] Fixed --simulate=diff to report permission and ownership changes The diff report only ran 'diff -u' on file contents, so a file whose mode, owner or group changed without its contents was listed with nothing under it. Permission, owner and group changes are now printed before the content diff. Ticket: ENT-14694 Changelog: Title (cherry picked from commit e55f9d8fefc488148739906bcff1bc33bd81b387) --- cf-agent/simulate_mode.c | 73 ++++++++++++++++++- .../29_simulate_mode/diff_mode.cf.expected | 1 + .../29_simulate_mode/diff_mode_ownership.cf | 63 ++++++++++++++++ .../diff_mode_ownership.cf.sub | 31 ++++++++ 4 files changed, 167 insertions(+), 1 deletion(-) create mode 100644 tests/acceptance/29_simulate_mode/diff_mode_ownership.cf create mode 100644 tests/acceptance/29_simulate_mode/diff_mode_ownership.cf.sub diff --git a/cf-agent/simulate_mode.c b/cf-agent/simulate_mode.c index dcd846a7cd5..982a337834f 100644 --- a/cf-agent/simulate_mode.c +++ b/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() { @@ -418,6 +423,71 @@ static bool RunDiff(const char *path1, const char *path2) return !failure; } +#ifndef __MINGW32__ +static inline void FormatUser(uid_t uid, char *buf, size_t buf_size) +{ + 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) { const char *chrooted_path = ToChangesChroot(path); @@ -455,6 +525,7 @@ static bool DiffFile(const char *path) switch (st_chrooted.st_mode & S_IFMT) { case S_IFREG: case S_IFDIR: + DiffStatInfo(path, &st_orig, &st_chrooted); return RunDiff(path, chrooted_path); default: printf("'%s' is a %s\n", path, GetFileTypeDescription(st_chrooted.st_mode)); diff --git a/tests/acceptance/29_simulate_mode/diff_mode.cf.expected b/tests/acceptance/29_simulate_mode/diff_mode.cf.expected index df7344380f2..7db2f11ae13 100644 --- a/tests/acceptance/29_simulate_mode/diff_mode.cf.expected +++ b/tests/acceptance/29_simulate_mode/diff_mode.cf.expected @@ -102,6 +102,7 @@ Only in WORKDIR/tmp/sub-dir/.: sub-file - +y =========================================================================== +'WORKDIR/tmp/perms' changed permissions from 0600 to 0000 =========================================================================== 'WORKDIR/tmp/transformer' no longer exists =========================================================================== diff --git a/tests/acceptance/29_simulate_mode/diff_mode_ownership.cf b/tests/acceptance/29_simulate_mode/diff_mode_ownership.cf new file mode 100644 index 00000000000..2fc379e4e76 --- /dev/null +++ b/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)"; +} diff --git a/tests/acceptance/29_simulate_mode/diff_mode_ownership.cf.sub b/tests/acceptance/29_simulate_mode/diff_mode_ownership.cf.sub new file mode 100644 index 00000000000..e0f2b603c33 --- /dev/null +++ b/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"; +} From c8ec7ff618182e470f103e169e62892883cf1547 Mon Sep 17 00:00:00 2001 From: Nick Anderson Date: Sat, 24 Oct 2026 10:47:43 -0500 Subject: [PATCH 2/3] Fixed --simulate=diff to show the new target of a replaced symlink When a symlink in the changes chroot replaced a link with a wrong target or an obstructing file, the diff report read the link from the real filesystem instead of from the changes chroot. A wrong link showed its old target with the first characters cut off, and an obstruction showed "Invalid target" because the original was a regular file. Ticket: ENT-14713 Changelog: Title (cherry picked from commit 8d6bc9ceb384c944822198e6c555e21aab42cc89) --- cf-agent/simulate_mode.c | 63 +++++++++---------- .../29_simulate_mode/diff_mode_symlinks.cf | 60 ++++++++++++++++++ .../diff_mode_symlinks.cf.sub | 15 +++++ 3 files changed, 104 insertions(+), 34 deletions(-) create mode 100644 tests/acceptance/29_simulate_mode/diff_mode_symlinks.cf create mode 100644 tests/acceptance/29_simulate_mode/diff_mode_symlinks.cf.sub diff --git a/cf-agent/simulate_mode.c b/cf-agent/simulate_mode.c index 982a337834f..1f5af93ed40 100644 --- a/cf-agent/simulate_mode.c +++ b/cf-agent/simulate_mode.c @@ -331,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; @@ -348,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); @@ -362,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]; @@ -490,10 +507,7 @@ static void DiffStatInfo(const char *path, const struct stat *st_orig, const str static bool DiffFile(const char *path) { - const char *chrooted_path = ToChangesChroot(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 @@ -502,38 +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: - DiffStatInfo(path, &st_orig, &st_chrooted); - 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() diff --git a/tests/acceptance/29_simulate_mode/diff_mode_symlinks.cf b/tests/acceptance/29_simulate_mode/diff_mode_symlinks.cf new file mode 100644 index 00000000000..c25cdd378b4 --- /dev/null +++ b/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) + ); +} diff --git a/tests/acceptance/29_simulate_mode/diff_mode_symlinks.cf.sub b/tests/acceptance/29_simulate_mode/diff_mode_symlinks.cf.sub new file mode 100644 index 00000000000..1cde3358e6b --- /dev/null +++ b/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"; +} From 58244a287197be04c41938dfada4985d27c57b4d Mon Sep 17 00:00:00 2001 From: Nick Anderson Date: Sat, 24 Oct 2026 10:47:44 -0500 Subject: [PATCH 3/3] Fixed --simulate symlink errors to show the real target, not a chroot path Symlinks mirrored into the changes chroot point into the chroot, so the "points to ... not ..." messages for a link with a wrong target showed the internal '/state/.changes/...' path instead of the target on the real filesystem. Ticket: ENT-14714 Changelog: Title (cherry picked from commit bfb47d7960d9157d8a38b75bafcf7e780710aff3) --- libpromises/files_links.c | 12 ++++- .../symlink_error_real_path.cf | 45 +++++++++++++++++++ .../symlink_error_real_path.cf.sub | 11 +++++ 3 files changed, 66 insertions(+), 2 deletions(-) create mode 100644 tests/acceptance/29_simulate_mode/symlink_error_real_path.cf create mode 100644 tests/acceptance/29_simulate_mode/symlink_error_real_path.cf.sub diff --git a/libpromises/files_links.c b/libpromises/files_links.c index 42752b69789..afcabc3e158 100644 --- a/libpromises/files_links.c +++ b/libpromises/files_links.c @@ -180,6 +180,14 @@ 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)) @@ -187,7 +195,7 @@ PromiseResult VerifyLink(EvalContext *ctx, char *destination, const char *source 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; } } diff --git a/tests/acceptance/29_simulate_mode/symlink_error_real_path.cf b/tests/acceptance/29_simulate_mode/symlink_error_real_path.cf new file mode 100644 index 00000000000..ce52375979a --- /dev/null +++ b/tests/acceptance/29_simulate_mode/symlink_error_real_path.cf @@ -0,0 +1,45 @@ +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"); +} + +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-14714" } + string => "Test that --simulate mode reports the real target of a wrong symlink, not its path in the changes chroot"; +} + +bundle agent check +{ + vars: + "command" + string => "$(sys.cf_agent) -Kf $(this.promise_filename).sub --simulate=diff"; + + methods: + "check" + usebundle => dcs_passif_output( + ".*\QLink '$(G.testroot)/wrong-target' points to '$(G.testroot)/target-old' not '$(G.testroot)/target-new', but not moving obstructions\E.*", + ".*points to[^\n]*\Q.changes/\E.*", + $(command), + $(this.promise_filename) + ); +} diff --git a/tests/acceptance/29_simulate_mode/symlink_error_real_path.cf.sub b/tests/acceptance/29_simulate_mode/symlink_error_real_path.cf.sub new file mode 100644 index 00000000000..003d7b35572 --- /dev/null +++ b/tests/acceptance/29_simulate_mode/symlink_error_real_path.cf.sub @@ -0,0 +1,11 @@ +body common control +{ + inputs => { "../default.cf.sub" }; +} + +bundle agent main +{ + files: + "$(G.testroot)/wrong-target" + link_from => linkfrom("$(G.testroot)/target-new", "symlink"); +}