ENT-10961, CFE-1840: Files promise can now modify immutable bit in file system attributes #5752

larsewi · 2025-04-01T12:52:32Z

TODO:

Is there any other ones that I've missed?

cf-bottom · 2025-04-02T10:54:35Z

Thank you for submitting a PR! Maybe @craigcomstock can review this?

libpromises/eval_context.c

libpromises/override_fsattrs.c

nickanderson · 2025-04-08T18:19:20Z

MMM, the transformer attribute comes to mind.

  bundle agent __main__
  {
    files:
        "/tmp/example.txt"
          content => "Hello World";
  
        "/tmp/example.txt"
          transformer => "/usr/bin/gzip $(this.promiser)";
  
    reports:
        "$(with)" with => join( ",", lsdir( "/tmp", "example.*", "true" ));
  }

Looking at the files promise docs I imagine that create would also be affected:

bundle agent __main__
{
  vars:
      "check_file" string => "/tmp/check-file.txt";

  files:
      "$(check_file)"
        handle => "create_file",
        create => "true",
        if => "file_absent_report_kept";

   reports:
      "$(check_file) does not exist"
        classes => results( "namespace", "file_absent_report" );

      "$(check_file) was repaired"
        depends_on => { "create_file" };

      "$(check_file) exists"
        if => fileexists( "/tmp/check-file.txt" );
}

R: /tmp/check-file.txt does not exist
R: /tmp/check-file.txt was repaired
R: /tmp/check-file.txt exists

And the delete attribute would seem to be affected as well.

And rename too

bundle agent __main__
{
  files:
    "/tmp/example.txt"
      content => "Disable me!";

    "/tmp/example.txt"
      rename => default:disable;
}
# For reference:
# body rename disable
# # @brief Disable the file
# {
#         disable => "true";
# }

   info: Created file '/tmp/example.txt', mode 0600
   info: Updated file '/tmp/example.txt' with content 'Disable me!'
warning: File object '/tmp/example.txt' exists, contrary to promise
   info: Changed permissions of '/tmp/example.txt' to 'mode 0600'
   info: Disabled file '/tmp/example.txt' by renaming to '/tmp/example.txt.cfdisabled' with mode 0600

I added them to the description.

libpromises/evalfunction.c

larsewi · 2025-06-02T11:30:52Z

@cf-bottom Jenkins with exotics please :)

cf-bottom · 2025-06-02T11:34:10Z

Alright, I triggered a build:

(with exotics)

Jenkins: https://ci.cfengine.com/job/pr-pipeline/12188/

Packages: http://buildcache.cfengine.com/packages/testing-pr/jenkins-pr-pipeline-12188/

The acl attribute of the files promise can now override the immutable bit. Ticket: ENT-10961, CFE-1840 Changelog: Commit Signed-off-by: Lars Erik Wik <[email protected]>

Signed-off-by: Lars Erik Wik <[email protected]>

Ticket: CFE-4529 Changelog: Title Signed-off-by: Lars Erik Wik <[email protected]>

Ticket: CFE-4529 Signed-off-by: Lars Erik Wik <[email protected]>

Ticket: ENT-10961, CFE-1840 Signed-off-by: Lars Erik Wik <[email protected]>

cf-bottom · 2025-06-13T10:20:05Z

Sure, I triggered a build:

(with exotics)

Jenkins: https://ci.cfengine.com/job/pr-pipeline/12284/

Packages: http://buildcache.cfengine.com/packages/testing-pr/jenkins-pr-pipeline-12284/

craigcomstock

Looks good! A high-level question about atomicity and maybe one typo. :)

libpromises/override_fsattrs.h

libpromises/override_fsattrs.c

craigcomstock · 2025-06-26T16:52:01Z

libpromises/override_fsattrs.c

+    }
+
+    /* We'll match the original file permissions on commit */
+    if (!CopyRegularFileDiskPerms(orig, copy, 0600))


I think you should change the orig file to immutable just before this copy. Not 100% atomic but pretty close so that between this Begin() and the Commit() no one else can "easily" modify the file without also modifying the immutable bit of course.

Not sure if this is a good idea. Let's discuss in digital F2F format :)

craigcomstock · 2025-06-26T17:57:27Z

libpromises/override_fsattrs.h

+ * @return false in case of failure
+ * @note The immutable bit is reset to it's original state
+ */
+bool OverrideImmutableCommit(


Here it seems that we should have something like:

OverrideImmutableBegin(struct utimbuf *times) to capture the modified/access times when we begin the "transaction"

Also, during this Begin() we should make the file immutable to attempt to restrict access as much as possible while we do "work" and before we call Commit().

And then OverrideImmutableCommit() takes that as a parameter and if the original file has changed mod/access since Begin() we should fail to commit saying that the file changed inside of the window of time of the transaction to modify the immutable file.

Also, during this Begin() we should make the file immutable to attempt to restrict access as much as possible while we do "work" and before we call Commit().

If we make the file immutable while another process is writing it, we can maybe cause it to be left in an inconsistent state?

I think this very quickly is becoming like the yak shaving. The task is to implement immutable bit support. Not to make CFEngine file operations atomic (they never have been). I created a value-add project CFE-4551 to try to achieve this.

All CFEngine file operations are supposed to be atomic.

All CFEngine file operations are supposed to be atomic.

They unfortunately are not. However, this code uses the same attempt to achieve "atomic" file operations as before. I.e., write changes to a temporary file and swap it with the original.

craigcomstock · 2025-06-26T17:58:37Z

cf-agent/verify_files.c

+
+    if (a.havefsattrs && a.fsattrs.haveimmutable && !a.fsattrs.immutable)
+    {
+        /* Here we only handle the clearing of the immutable the immutable


Suggested change

/* Here we only handle the clearing of the immutable the immutable

/* Here we only handle the clearing of the immutable

craigcomstock · 2025-06-26T18:00:06Z

cf-agent/verify_files.c

+
+    /* If we encounter any promises to mutate the file and the immutable
+     * attribute in body fsattrs is "true", we will override the immutable bit
+     * by temporarily clearing it when ever needed. */


Suggested change

* by temporarily clearing it when ever needed. */

* by temporarily clearing it whenever needed. */

craigcomstock · 2025-06-26T18:12:14Z

libpromises/evalfunction.c

+static const FnCallArg GET_ACLS_ARGS[] =
+{
+    {CF_ABSPATHRANGE, CF_DATA_TYPE_STRING, "Path to file or directory"},
+    {"default,access", CF_DATA_TYPE_OPTION, "Whether to get default or access ACL"},


So this is our "menu" item list for an attribute right? Is this required and if not, defaulted to "default"? I will probably find out later in the review. :)

If me make it optional and default to one of them, then we cannot add any more arguments in the future. Maybe this is OK?

Also it does not make sense to default to "default", since "default" only works on directories. "default" is like a template ACL that is inherited by all child objects.

vpodzime · 2025-06-27T07:54:43Z

olehermanse requested your review on this pull request.

😮‍💨 😅

olehermanse · 2025-06-27T15:07:47Z

😮‍💨 😅

@vpodzime I heard you like file systems and C code :)

vpodzime

Looks good to me in general. Three things that I'd change:

split the GetACL() stuff into a separate PR
do not add anything to EvalContext it's already in the attributes, a macro/inline function to get the boolean should do
git reset BEFORE_ALL_THESE_CHANGES and then add the final version of the utility functions as one commit and then one or multiple commits for supporting the set_immutable in all the attributes

vpodzime · 2025-06-30T11:40:48Z

cf-agent/verify_files.c

@@ -406,7 +406,7 @@ static PromiseResult VerifyFilePromise(EvalContext *ctx, char *path, const Promi
    /* If we encounter any promises to mutate the file and the immutable
     * attribute in body fsattrs is "true", we will override the immutable bit
     * by temporarily clearing it when ever needed. */
-    const bool override_immutable = a.havefsattrs && a.fsattrs.haveimmutable && a.fsattrs.immutable && is_immutable;
+    EvalContextOverrideImmutableSet(ctx, a.havefsattrs && a.fsattrs.haveimmutable && a.fsattrs.immutable && is_immutable);


It's already in the attributes and we do pass those into gazillions functions already, right?

It's not passed everywhere. We also would need to pass the is_immutable variable each function.

vpodzime · 2025-06-30T11:45:02Z

cf-agent/verify_files_utils.c

@@ -2000,7 +2000,7 @@ static bool TransformFile(EvalContext *ctx, char *file, const Attributes *attr,

    if (!IsExecutable(CommandArg0(BufferData(command))))
    {
-        RecordFailure(ctx, pp, attr, "Transformer '%s' for file '%s' failed", attr->transformer, file);
+        RecordFailure(ctx, pp, attr, " '%s' for file '%s' failed", attr->transformer, file);


Why this change?

Yup, I don't know how that happened. Maybe some typos in normal mode 😆

larsewi · 2025-07-01T11:37:50Z

Superseded by #5833

larsewi mentioned this pull request Apr 1, 2025

ENT-10961, CFE-1840: Files promise can now modify immutable bit in file system attributes #5694

Closed

larsewi force-pushed the override-immutable branch from bdd1918 to 69b1f52 Compare April 3, 2025 14:26

github-advanced-security bot found potential problems Apr 3, 2025

View reviewed changes

libpromises/eval_context.c Fixed Show fixed Hide fixed

libpromises/eval_context.c Fixed Show fixed Hide fixed

libpromises/override_fsattrs.c Fixed Show fixed Hide fixed

larsewi force-pushed the override-immutable branch 5 times, most recently from 06b3676 to 578286c Compare April 4, 2025 15:13

larsewi force-pushed the override-immutable branch 6 times, most recently from f82867d to 2f16047 Compare May 27, 2025 09:10

github-advanced-security bot found potential problems May 28, 2025

View reviewed changes

libpromises/evalfunction.c Fixed Show fixed Hide fixed

larsewi force-pushed the override-immutable branch 4 times, most recently from 7b81817 to 50ad347 Compare May 30, 2025 13:28

larsewi marked this pull request as ready for review May 30, 2025 15:08

larsewi force-pushed the override-immutable branch from 50ad347 to 2eec801 Compare June 2, 2025 07:54

larsewi requested review from craigcomstock and olehermanse June 3, 2025 07:47

larsewi force-pushed the override-immutable branch 3 times, most recently from b1c4763 to 0559bfe Compare June 3, 2025 09:20

larsewi added 19 commits June 13, 2025 12:17

acl attribute can now override immutable bit

1823cff

The acl attribute of the files promise can now override the immutable bit. Ticket: ENT-10961, CFE-1840 Changelog: Commit Signed-off-by: Lars Erik Wik <[email protected]>

evalfunction.c: Removed trailing whitespace

dab2d99

Signed-off-by: Lars Erik Wik <[email protected]>

Added policy function to get ACLs

983faa4

Ticket: CFE-4529 Changelog: Title Signed-off-by: Lars Erik Wik <[email protected]>

Added acceptance test for getacls()

4b6c26e

Ticket: CFE-4529 Signed-off-by: Lars Erik Wik <[email protected]>

Added acceptance test to set immutable bit

4053710

Ticket: ENT-10961, CFE-1840 Signed-off-by: Lars Erik Wik <[email protected]>

Added acceptance test to clear immutable bit

c99718d

Ticket: ENT-10961, CFE-1840 Signed-off-by: Lars Erik Wik <[email protected]>

Added acceptance test for immutable with content

5b13fea

Ticket: ENT-10961, CFE-1840 Signed-off-by: Lars Erik Wik <[email protected]>

Added acceptance test for immutable with copy_from

841a313

Ticket: ENT-10961, CFE-1840 Signed-off-by: Lars Erik Wik <[email protected]>

Added acceptance test for immutable with delete

f75ea3b

Ticket: ENT-10961, CFE-1840 Signed-off-by: Lars Erik Wik <[email protected]>

Added acceptance test for immutable with edit_line

8e85661

Ticket: ENT-10961, CFE-1840 Signed-off-by: Lars Erik Wik <[email protected]>

Added acceptance test for immutable with edit_xml

9f2b689

Ticket: ENT-10961, CFE-1840 Signed-off-by: Lars Erik Wik <[email protected]>

Added acceptance test for immutable with perms

23b6519

Ticket: ENT-10961, CFE-1840 Signed-off-by: Lars Erik Wik <[email protected]>

Added acceptance test for immutable with touch

6b0b70d

Ticket: ENT-10961, CFE-1840 Signed-off-by: Lars Erik Wik <[email protected]>

Added acceptance test for immutable with edit_template

587ed51

Ticket: ENT-10961, CFE-1840 Signed-off-by: Lars Erik Wik <[email protected]>

Added acceptance test for immutable with acl

4369f2f

Ticket: ENT-10961, CFE-1840 Signed-off-by: Lars Erik Wik <[email protected]>

Added acceptance test for immutable with transformer

40a4547

Ticket: ENT-10961, CFE-1840 Signed-off-by: Lars Erik Wik <[email protected]>

Added acceptance test for immutable with rename

b0ce49b

Ticket: ENT-10961, CFE-1840 Signed-off-by: Lars Erik Wik <[email protected]>

Added context to log messages in OverrideImmutableCommit

c61fe03

Ticket: ENT-10961, CFE-1840 Signed-off-by: Lars Erik Wik <[email protected]>

Make it more clear what the override argument does

7dc51ff

Ticket: ENT-10961, CFE-1840 Signed-off-by: Lars Erik Wik <[email protected]>

larsewi force-pushed the override-immutable branch from 13f8333 to 7dc51ff Compare June 13, 2025 10:17

olehermanse requested review from craigcomstock and vpodzime June 25, 2025 12:40

craigcomstock reviewed Jun 26, 2025

View reviewed changes

olehermanse removed their request for review June 27, 2025 15:06

vpodzime reviewed Jun 30, 2025

View reviewed changes

larsewi closed this Jul 1, 2025

	/* Here we only handle the clearing of the immutable the immutable
	/* Here we only handle the clearing of the immutable

	* by temporarily clearing it when ever needed. */
	* by temporarily clearing it whenever needed. */

ENT-10961, CFE-1840: Files promise can now modify immutable bit in file system attributes #5752

ENT-10961, CFE-1840: Files promise can now modify immutable bit in file system attributes #5752

Uh oh!

Conversation

larsewi commented Apr 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cf-bottom commented Apr 2, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nickanderson commented Apr 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

larsewi commented Jun 2, 2025

Uh oh!

cf-bottom commented Jun 2, 2025

Uh oh!

cf-bottom commented Jun 13, 2025 • edited by larsewi Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

craigcomstock left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vpodzime commented Jun 27, 2025

Uh oh!

olehermanse commented Jun 27, 2025

Uh oh!

vpodzime left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

larsewi commented Jul 1, 2025

Uh oh!

Uh oh!

larsewi commented Apr 1, 2025 •

edited

Loading

nickanderson commented Apr 8, 2025 •

edited

Loading

cf-bottom commented Jun 13, 2025 •

edited by larsewi

Loading