src/useradd.c: chroot or prefix SELinux file context

[ikerexxe]

Do not process SELinux file context during file closure when chroot or
prefix options are selected.

As I'm changing a lot of files I decided to split the changes in a set of
patches to make them easier to understand.

Tests: #940
Closes: #940

[ikerexxe]

This is blocked by next-actions/pytest-mh#101

[ikerexxe]

@praiskup and @pmatilai I have created a COPR repository to help test these changes. Do you mind testing them?

By the way, if you need the build in some other distribution let me know.

[pmatilai]

Hmm, I'm afraid I don't see any behavior change from this on the rpm case (this on Fedora 41'ish):

[root@lumikko tmp]# rpm -q shadow-utils
shadow-utils-4.17.4-30test.fc41.x86_64
[root@lumikko tmp]# rm -rf /srv/test; rpm -U --root /srv/test/ setup-2.15.0-8.fc41.noarch.rpm --nodeps --nosignature --define "__systemd_sysusers /usr/lib/rpm/sysusers.sh"
groupadd: failure while writing changes to /etc/group
groupadd: failure while writing changes to /etc/group
groupadd: failure while writing changes to /etc/group
groupadd: failure while writing changes to /etc/group
groupadd: failure while writing changes to /etc/group
groupadd: failure while writing changes to /etc/group
groupadd: failure while writing changes to /etc/group
groupadd: failure while writing changes to /etc/group
groupadd: failure while writing changes to /etc/group
groupadd: failure while writing changes to /etc/group
groupadd: failure while writing changes to /etc/group
groupadd: failure while writing changes to /etc/group
groupadd: failure while writing changes to /etc/group
groupadd: failure while writing changes to /etc/group
groupadd: failure while writing changes to /etc/group
groupadd: failure while writing changes to /etc/group
groupadd: failure while writing changes to /etc/group
groupadd: failure while writing changes to /etc/group
groupadd: failure while writing changes to /etc/group
groupadd: failure while writing changes to /etc/group
groupadd: failure while writing changes to /etc/group
groupadd: failure while writing changes to /etc/group
groupadd: failure while writing changes to /etc/group
groupadd: failure while writing changes to /etc/group
groupadd: failure while writing changes to /etc/group
groupadd: failure while writing changes to /etc/group
groupadd: failure while writing changes to /etc/group
groupadd: failure while writing changes to /etc/group
groupadd: failure while writing changes to /etc/group
useradd: group '4' does not exist
useradd: group '1' does not exist
useradd: group '2' does not exist
useradd: group '50' does not exist
useradd: group '100' does not exist
useradd: group '0' does not exist
useradd: group '7' does not exist
useradd: group '12' does not exist
useradd: group '65534' does not exist
useradd: group '0' does not exist
useradd: group '0' does not exist
useradd: group '0' does not exist
useradd: group '0' does not exist
[root@lumikko tmp]#

That's how it fails with the stock F41 shadow-utils-4.15.1-12.fc41.x86_64 too.

[ikerexxe]

I wasn't aware that the specific problem you were facing included groupadd, so I only updated the APIs and useradd as I wanted to confirm this was the way forward before updating other tools.

With this new information I have changed the groupadd code to avoid relabeling any file. I have tested this code with the command you provided and it seems to be working. I had to disable SELinux though, as I was hitting an AVC denial when trying to open the chroot group file.

I have updated the build COPR repository to include these changes. Test it when you can, and if you run into any problems share the exact steps you used so I can reproduce it.

[praiskup]

Tested with the proposed chagnes, and I can create users/groups with the --root. I still can't remove them (prefix works):

[root@vm-10-0-185-185 ~]# userdel -f mockbuild --root /var/lib/mock/fedora-41-x86_64/root
userdel: failure while writing changes to /etc/passwd
[root@vm-10-0-185-185 ~]# cat /var/lib/mock/fedora-41-x86_64/root/etc/passwd | grep mockbuild
mockbuild:x:1001:135::/builddir:/bin/bash
[root@vm-10-0-185-185 ~]# ls -alhZ /var/lib/mock/fedora-41-x86_64/root/etc/passwd
-rw-r--r--. 1 root root unconfined_u:object_r:mock_var_lib_t:s0 617 May 30 07:09 /var/lib/mock/fedora-41-x86_64/root/etc/passwd
[root@vm-10-0-185-185 ~]# userdel -f mockbuild --prefix /var/lib/mock/fedora-41-x86_64/root
[root@vm-10-0-185-185 ~]# echo $?
0

[ikerexxe]

That's expected behaviour as I only proposed the fix for useradd and groupadd binaries, everything else isn't fixed yet.

@alejandro-colomar @hallyn would you mind reviewing the general concept of this PR? You can skip the testing and just review how I propose to handle the propagation of the process_selinux flag from the various binaries to the library that handles file management. There are already quite a few commits in this PR and making all the binaries behave the same way is going to increase them even more, so I would like to be sure that this is the best way to solve this problem before proceeding with the rest.

[alejandro-colomar]

That's expected behaviour as I only proposed the fix for useradd and groupadd binaries, everything else isn't fixed yet.

@alejandro-colomar @hallyn would you mind reviewing the general concept of this PR? You can skip the testing and just review how I propose to handle the propagation of the process_selinux flag from the various binaries to the library that handles file management. There are already quite a few commits in this PR and making all the binaries behave the same way is going to increase them even more, so I would like to be sure that this is the best way to solve this problem before proceeding with the rest.

Yep, I'll review.

[pmatilai]

Doh, I've missed the update round here. I'll try to retest soon, thanks for looking into this!

[alejandro-colomar]

The changes seem relatively simple. I ignore SELinux, so I can't review the idea, but the code seems reasonable.

[alejandro-colomar]

The changes seem relatively simple. I ignore SELinux, so I can't review the idea, but the code seems reasonable.

It would be interesting to merge this PR as a proper merge commit instead of a rebase, to keep it organized as a single block of changes, BTW.

[ikerexxe]

This PR is ready for review and testing. The COPR repository has also been updated with the latest changes. @praiskup and @pmatilai do you mind testing?

I've updated all binaries to prevent SELinux file context processing during file closure when chroot or prefix options are selected. Subordinate IDs library hasn't been updated due to API changes and the low probability of SELinux usage within chroot for subIDs, thus in this case the SELinux file context will be processed.

System tests have been removed as this PR already includes significant changes and container compatibility issues prevent proper execution.

[hallyn]

I'm ok with this.

I do wonder whether we would be better off using some ambient state (maybe just a simple global) to keep track of the process_selinux bool. Have it automatically set to true at the start of each (relevant) program, and have the chroot and prefix options, when parsed, set it to false.

It would keep a lot of the function signatures simpler, but I'm not sure the end result is better, so feel free to merge this.

[alejandro-colomar]

I'm ok with this.

I do wonder whether we would be better off using some ambient state (maybe just a simple global) to keep track of the process_selinux bool. Have it automatically set to true at the start of each (relevant) program, and have the chroot and prefix options, when parsed, set it to false.

It would keep a lot of the function signatures simpler, but I'm not sure the end result is better, so feel free to merge this.

I prefer passing it locally. While a static looks simpler, it might complicate things later. I'm working on removing some statics elsewhere, and I think it would be good to not add more.

[alejandro-colomar]

@ikerexxe You need to rebase a few conflicts.

[ikerexxe]

I prefer passing it locally. While a static looks simpler, it might complicate things later. I'm working on removing some statics elsewhere, and I think it would be good to not add more.

I also prefer to pass it locally and add yet another global variable that will complicate things in the future.

@ikerexxe You need to rebase a few conflicts.

I'll do it during the week.

By the way, @pmatilai @praiskup I'm waiting for you to test this.

[ikerexxe]

After or before the rebase?

It doesn't really matter because the code has changed quite a bit between shadow 4.18 and master, so I would have to port a few commits to the COPR build before I could apply this branch cleanly.

Before the rebase, great! It works, thank you.

For my part, these tests would be sufficient. Thank you for checking.

[hallyn]

@ikerexxe so we're good to merge?

[ikerexxe]

@hallyn yes, I think this is good as it is. Please keep in mind Alejandro's comment:

It would be interesting to merge this PR as a proper merge commit instead of a rebase, to keep it organized as a single block of changes, BTW.