ENT-10961, CFE-1840: Files promise can now modify immutable bit in file system attributes by larsewi · Pull Request #5752 · cfengine/core

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?

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.

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/

Signed-off-by: Lars Erik Wik <lars.erik.wik@northern.tech>

Ticket: CFE-4529 Changelog: Title Signed-off-by: Lars Erik Wik <lars.erik.wik@northern.tech>

Ticket: CFE-4529 Signed-off-by: Lars Erik Wik <lars.erik.wik@northern.tech>

Ticket: ENT-10961, CFE-1840 Signed-off-by: Lars Erik Wik <lars.erik.wik@northern.tech>

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. :)

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

+    }
+
+    /* 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

+ * @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

+
+    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

+
+    /* 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

+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

     * 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

    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 AI found potential problems Apr 3, 2025

View reviewed changes

Comment thread libpromises/eval_context.c Fixed

Comment thread libpromises/eval_context.c Fixed

Comment thread libpromises/override_fsattrs.c 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 AI found potential problems May 28, 2025

View reviewed changes

Comment thread libpromises/evalfunction.c 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 18 commits June 13, 2025 12:17

evalfunction.c: Removed trailing whitespace

dab2d99

Signed-off-by: Lars Erik Wik <lars.erik.wik@northern.tech>

Added policy function to get ACLs

983faa4

Ticket: CFE-4529 Changelog: Title Signed-off-by: Lars Erik Wik <lars.erik.wik@northern.tech>

Added acceptance test for getacls()

4b6c26e

Ticket: CFE-4529 Signed-off-by: Lars Erik Wik <lars.erik.wik@northern.tech>

Added acceptance test to set immutable bit

4053710

Ticket: ENT-10961, CFE-1840 Signed-off-by: Lars Erik Wik <lars.erik.wik@northern.tech>

Added acceptance test to clear immutable bit

c99718d

Ticket: ENT-10961, CFE-1840 Signed-off-by: Lars Erik Wik <lars.erik.wik@northern.tech>

Added acceptance test for immutable with content

5b13fea

Ticket: ENT-10961, CFE-1840 Signed-off-by: Lars Erik Wik <lars.erik.wik@northern.tech>

Added acceptance test for immutable with copy_from

841a313

Ticket: ENT-10961, CFE-1840 Signed-off-by: Lars Erik Wik <lars.erik.wik@northern.tech>

Added acceptance test for immutable with delete

f75ea3b

Ticket: ENT-10961, CFE-1840 Signed-off-by: Lars Erik Wik <lars.erik.wik@northern.tech>

Added acceptance test for immutable with edit_line

8e85661

Ticket: ENT-10961, CFE-1840 Signed-off-by: Lars Erik Wik <lars.erik.wik@northern.tech>

Added acceptance test for immutable with edit_xml

9f2b689

Ticket: ENT-10961, CFE-1840 Signed-off-by: Lars Erik Wik <lars.erik.wik@northern.tech>

Added acceptance test for immutable with perms

23b6519

Ticket: ENT-10961, CFE-1840 Signed-off-by: Lars Erik Wik <lars.erik.wik@northern.tech>

Added acceptance test for immutable with touch

6b0b70d

Ticket: ENT-10961, CFE-1840 Signed-off-by: Lars Erik Wik <lars.erik.wik@northern.tech>

Added acceptance test for immutable with edit_template

587ed51

Ticket: ENT-10961, CFE-1840 Signed-off-by: Lars Erik Wik <lars.erik.wik@northern.tech>

Added acceptance test for immutable with acl

4369f2f

Ticket: ENT-10961, CFE-1840 Signed-off-by: Lars Erik Wik <lars.erik.wik@northern.tech>

Added acceptance test for immutable with transformer

40a4547

Ticket: ENT-10961, CFE-1840 Signed-off-by: Lars Erik Wik <lars.erik.wik@northern.tech>

Added acceptance test for immutable with rename

b0ce49b

Ticket: ENT-10961, CFE-1840 Signed-off-by: Lars Erik Wik <lars.erik.wik@northern.tech>

Added context to log messages in OverrideImmutableCommit

c61fe03

Ticket: ENT-10961, CFE-1840 Signed-off-by: Lars Erik Wik <lars.erik.wik@northern.tech>

Make it more clear what the override argument does

7dc51ff

Ticket: ENT-10961, CFE-1840 Signed-off-by: Lars Erik Wik <lars.erik.wik@northern.tech>

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

larsewi deleted the override-immutable branch July 25, 2025 10:54

	/* 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. */

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!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

7 participants

larsewi commented Apr 1, 2025 •

edited

Loading

nickanderson commented Apr 8, 2025 •

edited

Loading

cf-bottom commented Jun 13, 2025 •

edited by larsewi

Loading