CFE-3725: fix double change detection with hash=>"best"

cf-agent/verify_files_utils.c

@@ -3690,35 +3690,29 @@ static PromiseResult VerifyFileIntegrity(EvalContext *ctx, const char *file, con
 {
     assert(attr != NULL);
     unsigned char digest1[EVP_MAX_MD_SIZE + 1];
-    unsigned char digest2[EVP_MAX_MD_SIZE + 1];
 
     if ((attr->change.report_changes != FILE_CHANGE_REPORT_CONTENT_CHANGE) && (attr->change.report_changes != FILE_CHANGE_REPORT_ALL))
     {
         return PROMISE_RESULT_NOOP;
     }
 
     memset(digest1, 0, EVP_MAX_MD_SIZE + 1);
-    memset(digest2, 0, EVP_MAX_MD_SIZE + 1);
 
     PromiseResult result = PROMISE_RESULT_NOOP;
     bool changed = false;
-    if (attr->change.hash == HASH_METHOD_BEST)
-    {
-        HashFile(file, digest1, HASH_METHOD_MD5, false);
-        HashFile(file, digest2, HASH_METHOD_SHA1, false);
 
-        changed = (changed ||
-                   FileChangesCheckAndUpdateHash(ctx, file, digest1, HASH_METHOD_MD5, attr, pp, &result));
-        changed = (changed ||
-                   FileChangesCheckAndUpdateHash(ctx, file, digest2, HASH_METHOD_SHA1, attr, pp, &result));
-    }
-    else
-    {
-        HashFile(file, digest1, attr->change.hash, false);
+    /* "best" resolves to a single strong hash (SHA-512) in both Community and
+     * Enterprise (see GetBestFileChangeHashMethod()), so HASH_METHOD_BEST is
+     * never a runtime hash value here. Previously the Community stub returned
+     * HASH_METHOD_BEST and this function checked MD5 and SHA1 in sequence with
+     * a short-circuiting ||, which skipped updating the second hash once the
+     * first reported a change -- leaving it stale and firing a phantom
+     * "Content changed" on the next run (CFE-3725). */
+    assert(attr->change.hash != HASH_METHOD_BEST);
 
-        changed = (changed ||
-                   FileChangesCheckAndUpdateHash(ctx, file, digest1, attr->change.hash, attr, pp, &result));
-    }
+    HashFile(file, digest1, attr->change.hash, false);
+
+    changed = FileChangesCheckAndUpdateHash(ctx, file, digest1, attr->change.hash, attr, pp, &result);
 
     if (changed && MakingInternalChanges(ctx, pp, attr, &result, "record integrity changes in '%s'", file))
     {

libpromises/attributes.c

@@ -876,7 +876,10 @@ FileRename GetRenameConstraints(const EvalContext *ctx, const Promise *pp)
 
 ENTERPRISE_FUNC_0ARG_DEFINE_STUB(HashMethod, GetBestFileChangeHashMethod)
 {
-    return HASH_METHOD_BEST;
+    /* "best" resolves to the strongest available hash (SHA-512), matching the
+     * Enterprise implementation of this function. The legacy MD5+SHA1 pairing
+     * was neither the "best" pair of algorithms nor FIPS-approved. */
+    return HASH_METHOD_SHA512;
 }
 
 FileChange GetChangeMgtConstraints(const EvalContext *ctx, const Promise *pp)

tests/acceptance/10_files/02_maintain/changes_best_hash_no_double_detection.cf

@@ -0,0 +1,103 @@
+# Test that a files 'changes' body with hash => "best" reports a content change
+# exactly ONCE, with no phantom re-detection on the following run (CFE-3725).
+#
+# hash => "best" now resolves to a single strong hash (SHA-512). Previously
+# Community "best" checked MD5 and SHA1 in sequence with a short-circuit: when
+# MD5 detected a change the SHA1 update was skipped, leaving the stored SHA1
+# stale, so the NEXT run fired a second "Content changed" via the SHA1 check.
+# With a single hash a content change is logged exactly once.
+#
+# Runs, all inside one agent invocation:
+#   init   - baseline an (empty) file with hash => "best"
+#   test   - change the content, then re-check (run 2), then re-check again (run 3)
+#   check  - the change log must contain EXACTLY ONE "Content changed" for the file
+#            (before the fix there are two: run 2 real + run 3 phantom).
+body common control
+{
+  inputs => { "../../default.sub.cf", "check_file_changes_log.cf.sub" };
+  bundlesequence => { default("$(this.promise_filename)") };
+  version => "1.0";
+}
+
+bundle agent init
+{
+  files:
+    # Baseline the file with hash => "best" (records the stored hash).
+    "$(G.testfile).best"
+      create => "true",
+      changes => changes_best;
+}
+
+bundle agent test
+{
+  meta:
+    "test_skip_unsupported" string => "windows";
+
+  files:
+    # Change the content so the next check detects a difference.
+    "$(G.testfile).best" edit_line => insert_marker;
+
+  methods:
+    # Run 2: re-check the now-changed file -> reports the change (once).
+    "run2" usebundle => recheck_best;
+
+    # Run 3: CFEngine skips a bundle it already ran, so a duplicate bundle
+    # gives us a third run. No further content change -> must report nothing.
+    "run3" usebundle => recheck_best_again;
+}
+
+bundle edit_line insert_marker
+{
+  insert_lines:
+    "changed content for CFE-3725";
+}
+
+bundle agent recheck_best
+{
+  files:
+    "$(G.testfile).best" changes => changes_best;
+}
+
+bundle agent recheck_best_again
+{
+  files:
+    "$(G.testfile).best" changes => changes_best;
+}
+
+body changes changes_best
+{
+  hash => "best";
+  report_changes => "content";
+  update_hashes => "yes";
+}
+
+bundle agent check
+{
+  methods:
+    # Expected: exactly one "Content changed" entry for our file. check_file_changes_log
+    # reads the real $(sys.workdir)/state/file_changes.log, strips unstable fields
+    # (timestamp/promise-id/path) down to "<leafname>,C,Content changed", and diffs.
+    "any"
+      usebundle => file_make(
+        "$(G.testfile).expected", "TEST.cfengine.best,C,Content changed"
+      );
+
+    "any"
+      usebundle => check_file_changes_log(
+        "$(G.testfile).expected", "cfe3725_log_ok", "cfe3725_log_fail", ""
+      );
+
+  reports:
+    # Decide the outcome from the classes that check_file_changes_log sets in
+    # the methods promises above. Guarding the reports directly (instead of
+    # via an intermediate "ok" class in a classes promise, which would be
+    # evaluated a pass earlier) means exactly one of Pass/FAIL is emitted.
+    cfe3725_log_ok.!cfe3725_log_fail::
+      "$(this.promise_filename) Pass";
+
+    cfe3725_log_fail|!cfe3725_log_ok::
+      "$(this.promise_filename) FAIL: expected exactly one 'Content changed' for the file (hash=>best must not re-detect on the next run)";
+}
+
+### PROJECT_ID: core
+### CATEGORY_ID: 27

tests/acceptance/28_inform_testing/01_files/changes01.cf.expected

@@ -1,2 +1 @@
-    info: Stored md5 hash for '/tmp/TEST.cfengine' (MD5=d41d8cd98f00b204e9800998ecf8427e)
-    info: Stored sha1 hash for '/tmp/TEST.cfengine' (SHA=da39a3ee5e6b4b0d3255bfef95601890afd80709)
+    info: Stored sha512 hash for '/tmp/TEST.cfengine' (SHA=cf83e1357eefb8bdf1542850d66d8007d620e4050b5715dc83f4a921d36ce9ce47d0d13c5d85f2b0ff8318d2877eec2f63b931bd47417a81a538327af927da3e)