Set acl permissions on workdir root to ensure multi-user access when umask is 0027

[carlosrodfern]

In machines with umask set to 0027 it is necessary to setup an acl to overide it so the workdir root directory stays multi-user access. This is specially necessary on machines accessed with a non-root user, that are hardened to CIS Level 1 guidelines.

It is happening in version 1.29.0.

Fixes: #2496

Pull Request Checklist

• implement the feature
• write the documentation (docstrings)
• extend the test coverage
• update the specification (N/A)
• adjust plugin docstring
• modify the json schema (N/A)
• mention the version
• include a release note (the PR title)

[carlosrodfern]

This MR is just a draft, to bring this conceptual fix to the referred issue. There is some cleanup to do. I'm OK with a complete change of approach or even if this is not worth the fix if there is a workaround. That being said hardened machines are so common that the fix may actually be worth doing.

It fixes the issue as seen in the STR.

Thoughts?

[carlosrodfern]

@happz @lukaszachy , overall, is this a good approach I should continue? setting up acl permissions globally at initialization time?

Other brainstroming ideas: setting up acl per plan workdir, or making sure the output files being generated are all with multi-user permissions from the beginning as they are being generated.

[lukaszachy]

@carlosrodfern Thanks for the draft, but unfortunately I accumulated some other work I need to finish first. I'll take a better look next week.

IMO we don't need to be too specific here so setting it globally should be OK. I see no point in hardening /var/tmp/tmt on throwaway testing machine.

[carlosrodfern]

@happz , I believe addressed all reviews. I also went thru the checklist, and added tests. If I missed something please let me know. Thank you for reviewing this PR.

[happz]

@happz , I believe addressed all reviews. I also went thru the checklist, and added tests. If I missed something please let me know. Thank you for reviewing this PR.

It seems to me another patch of yours sneaked into this PR, #2659. It makes the review more complicated, but other than that...

@psss this PR was missing a milestone, would it still fit into 1.31 schedule?

[carlosrodfern]

@happz , I believe addressed all reviews. I also went thru the checklist, and added tests. If I missed something please let me know. Thank you for reviewing this PR.

It seems to me another patch of yours sneaked into this PR, #2659. It makes the review more complicated, but other than that...

@psss this PR was missing a milestone, would it still fit into 1.31 schedule?

I separated the basic test for virtual provision into its own PR to get that one done sooner, and because it is indeed not directly related to this PR. That way the only test this PR would bring is the umask one.
I also assumed you all needed time to look into this PR more carefully.

The PR that gets merged first will create a minor conflict for the other, but I'll fix it quickly for the second PR being merged.

[happz]

@happz , I believe addressed all reviews. I also went thru the checklist, and added tests. If I missed something please let me know. Thank you for reviewing this PR.

It seems to me another patch of yours sneaked into this PR, #2659. It makes the review more complicated, but other than that...
@psss this PR was missing a milestone, would it still fit into 1.31 schedule?

I separated the basic test for virtual provision into its own PR to get that one done sooner, and because it is indeed not directly related to this PR. That way the only test this PR would bring is the umask one. I also assumed you all needed time to look into this PR more carefully.

The PR that gets merged first will create a minor conflict for the other, but I'll fix it quickly for the second PR being merged.

I see. In that case, I would recommend merging #2659 first because it contains just a single change, unlike this PR.

[carlosrodfern]

@happz , I believe addressed all reviews. I also went thru the checklist, and added tests. If I missed something please let me know. Thank you for reviewing this PR.

It seems to me another patch of yours sneaked into this PR, #2659. It makes the review more complicated, but other than that...
@psss this PR was missing a milestone, would it still fit into 1.31 schedule?

I separated the basic test for virtual provision into its own PR to get that one done sooner, and because it is indeed not directly related to this PR. That way the only test this PR would bring is the umask one. I also assumed you all needed time to look into this PR more carefully.
The PR that gets merged first will create a minor conflict for the other, but I'll fix it quickly for the second PR being merged.

I see. In that case, I would recommend merging #2659 first because it contains just a single change, unlike this PR.

The pull request #2659 has been merged. I rebased this PR, fixed some bug, setup the test based on the new tests, and pushed an updated commit.

[psss]

Thanks for working on this. Let's try to squeeze this into 1.32.

[happz]

I still don't like the fact plugin developer needs to call setup() explicitly, it's easy to forget. But we can fix that later (see #2510 (comment)).

[carlosrodfern]

I still don't like the fact plugin developer needs to call setup() explicitly, it's easy to forget. But we can fix that later (see #2510 (comment)).

One idea that can help is to explicitly model the state machine this represents, with a variable or object representing the state, and then action functions (e.g., start, setup, wake, stop, ...), that check preconditions like the current state, and transition it. That way you have code ensuring things are done in the right order of steps. Then you can add helper functions (e.g., get_ready) that represent a "desired state" that help transition the state machine several states by calling the action s functions in the right order.

Representing that state machine can help with readability for the developer, and correctness in general, by making the state explicit, and organizing the functions around that concept. Right now the state machine is implicitly represented in the values of the Guest properties. I think the current code is already very close.

[happz]

I still don't like the fact plugin developer needs to call setup() explicitly, it's easy to forget. But we can fix that later (see #2510 (comment)).

One idea that can help is to explicitly model the state machine this represents, with a variable or object representing the state, and then action functions (e.g., start, setup, wake, stop, ...), that check preconditions like the current state, and transition it. That way you have code ensuring things are done in the right order of steps. Then you can add helper functions (e.g., get_ready) that represent a "desired state" that help transition the state machine several states by calling the action s functions in the right order.

Representing that state machine can help with readability for the developer, and correctness in general, by making the state explicit, and organizing the functions around that concept. Right now the state machine is implicitly represented in the values of the Guest properties. I think the current code is already very close.

I would go even further, with a typestate pattern where the state of an object is expressed by its type (e.g. https://cliffle.com/blog/rust-typestate/#a-simple-example-the-living-and-the-dead). TL;DR, "running" guest would be a different type than "not running". A running guest type would not even have methods like start(), "not running" guest would not have stop(), and so on. Once wake() or setup() get called, the proper type would be returned, something like DeadGuest: start(self) -> RunningGuest, RunningGuest: def stop(self) -> DeadGuest. Methods now accepting Guest would change their signatures to request the hypothetical RunningAndReadyGuest type only, plugins' go() method would return RunningAndReadyGuest instances instead of None, and RunningGuest would have setup() -> RunningAndReadyGuest method which would force a developer to call Guest.start() first to obtain RunningGuest, followed by call to its setup() to obtain RunningAndReadyGuest type that can be returned.

I already tried this out, it does not look that bad or complex, although I'm still far from a reviewable patch. But I liked the approach very much, it would detect violations in pre-commit time.

[psss]

Looks good. Thanks for fixing this!

[psss]

/packit test