#2717 PR `closed`: Add a prerelabel with setfiles before the first reboot, because autorelabel is not always reliable¶

Labels: enhancement, no-pr-activity

bwelterl opened issue at 2021-11-19 14:26:¶

Starting in RHEL8.4, the selinux policy does not allow systemd to access unlabeled files anymore, thus if the restored filesystem has not the correct labels (for example /etc/localtime), systemd will not be able to boot and relabel will fail and system will not boot.

Relax-and-Recover (ReaR) Pull Request Template¶

Please fill in the following items before submitting a new pull request:

Pull Request Details:¶

Type: Bug Fix / New Feature / Enhancement / Other?
Enhancement
Impact: Low / Normal / High / Critical / Urgent
Normal
Reference to related issue (URL):
https://github.com/rear/rear/issues/2716
How was this pull request tested?

A VM with a wrong label for /etc/localtime is restored correctly thanks to setfiles (and the relabel is made at reboot)

Brief description of the changes in this pull request:

add setfiles on /mnt/local before the first reboot
we need to add PROGS+=( setfiles ) in local.conf. Need to be discussed where to add this dependency.

pcahyna commented at 2021-11-19 14:57:¶

Hello @bwelterl , thanks for the patch. Would it make sense to run setfiles in the /mnt/local chroot? This way you would not need to add PROGS+=( setfiles ). I believe that it is common to run such post-restore fixups in the chroot.

bwelterl commented at 2021-11-19 15:07:¶

Hello,
I was thinking about this.
I will test it, it will be easier and no need to add modification in the backup phase.

Thanks

bwelterl commented at 2021-11-22 08:56:¶

Hello @bwelterl , thanks for the patch. Would it make sense to run setfiles in the /mnt/local chroot? This way you would not need to add PROGS+=( setfiles ). I believe that it is common to run such post-restore fixups in the chroot.

Thanks @pcahyna
I changed the code to execute in the chroot. It seems to work well.
Thanks

hpannenb commented at 2021-11-22 18:10:¶

Hi, @bwelterl . If You do not mind I noticed two minor typos. Maybe "This can take several minutes."
https://github.com/rear/rear/blob/7609a9c7c360eab1ce84b2766a474b38e9434ace/usr/share/rear/restore/NETFS/default/500_selinux_autorelabel.sh#L27
and "accessible" instead.
https://github.com/rear/rear/blob/7609a9c7c360eab1ce84b2766a474b38e9434ace/usr/share/rear/restore/NETFS/default/500_selinux_autorelabel.sh#L33

pcahyna commented at 2021-11-23 10:33:¶

Concerning extended attributes in general and specifically in rsync, see https://github.com/rear/rear/pull/2577#issuecomment-840743973 :

Not that the rsync code is entirely correct: it does not restore extended attributes: so before restore you have
$ getfattr -d -m- /usr/bin/ping
getfattr: Removing leading '/' from absolute path names
# file: usr/bin/ping
security.capability=0sAAAAAgAwAAAAAAAAAAAAAAAAAAA=
security.selinux="system_u:object_r:ping_exec_t:s0"
while after restore just
getfattr -d -m- /usr/bin/ping
getfattr: Removing leading '/' from absolute path names
# file: usr/bin/ping
security.selinux="system_u:object_r:ping_exec_t:s0"
so the system will be a bit broken in different ways.

So, while your change makes sense for TSM, I suspect that it is only papering over a deeper problem for RSYNC. I will look at solving the problem in more generic ways for RSYNC.
In the meantime, can you please try the getfattr test above for the restored systems, especially with TSM, as I don't have access to TSM? I would like to know whether the TSM problems are limited to SELinux attributes.

jsmeix commented at 2021-11-23 14:08:¶

I know nothing about SELinux so I may misunderstand things
but I wonder why the same code needs to be run twice here?

The added code in
usr/share/rear/restore/NETFS/default/500_selinux_autorelabel.sh
and
usr/share/rear/restore/default/500_selinux_autorelabel.sh
is same except whitespaces:

@@ -9 +9 @@
-    This can take a several minutes."
+        This can take a several minutes."
@@ -12 +12 @@
-     #setfiles -c $TARGET_FS_ROOT/etc/selinux/${policy}/policy/policy.*  $TARGET_FS_ROOT/etc/selinux/${policy}/contexts/files/file_contexts
+     # setfiles -c $TARGET_FS_ROOT/etc/selinux/${policy}/policy/policy.*  $TARGET_FS_ROOT/etc/selinux/${policy}/contexts/files/file_contexts
@@ -15,2 +15,2 @@
-    LogPrint "The configured selinux policy $policy is not accessbible in default path $TARGET_FS_ROOT/etc/selinux/${policy}/. \n
-    If the first boot fails, please add 'enforcing=0' on kernel command line, and an autorelabel should fix the labels."
+     LogPrint "The configured selinux policy $policy is not accessbible in default path $TARGET_FS_ROOT/etc/selinux/${policy}/. \n
+     If the first boot fails, please add 'enforcing=0' on kernel command line, and an autorelabel should fix the labels."

diff -w shows no differences in that code.

At least for me with BACKUP=NETFS both
usr/share/rear/restore/NETFS/default/500_selinux_autorelabel.sh
and
usr/share/rear/restore/default/500_selinux_autorelabel.sh
are run

# usr/sbin/rear -s recover | grep selinux
Source restore/NETFS/default/500_selinux_autorelabel.sh
Source restore/default/500_selinux_autorelabel.sh
Source restore/NETFS/Linux-i386/510_selinux_fixfiles_exclude_dirs.sh

so I wonder it it wouldn't be simpler to have that code only in
usr/share/rear/restore/default/500_selinux_autorelabel.sh
(perhaps with some additional conditions to run it there)?

bwelterl commented at 2021-11-23 14:11:¶

I know nothing about SELinux so I may misunderstand things but I wonder why the same code needs to be run twice here?

The added code in usr/share/rear/restore/NETFS/default/500_selinux_autorelabel.sh and usr/share/rear/restore/default/500_selinux_autorelabel.sh is same except whitespaces:
@@ -9 +9 @@
-    This can take a several minutes."
+        This can take a several minutes."
@@ -12 +12 @@
-     #setfiles -c $TARGET_FS_ROOT/etc/selinux/${policy}/policy/policy.*  $TARGET_FS_ROOT/etc/selinux/${policy}/contexts/files/file_contexts
+     # setfiles -c $TARGET_FS_ROOT/etc/selinux/${policy}/policy/policy.*  $TARGET_FS_ROOT/etc/selinux/${policy}/contexts/files/file_contexts
@@ -15,2 +15,2 @@
-    LogPrint "The configured selinux policy $policy is not accessbible in default path $TARGET_FS_ROOT/etc/selinux/${policy}/. \n
-    If the first boot fails, please add 'enforcing=0' on kernel command line, and an autorelabel should fix the labels."
+     LogPrint "The configured selinux policy $policy is not accessbible in default path $TARGET_FS_ROOT/etc/selinux/${policy}/. \n
+     If the first boot fails, please add 'enforcing=0' on kernel command line, and an autorelabel should fix the labels."
At least for me with BACKUP=NETFS both usr/share/rear/restore/NETFS/default/500_selinux_autorelabel.sh and usr/share/rear/restore/default/500_selinux_autorelabel.sh are run
# usr/sbin/rear -s recover | grep selinux
Source restore/NETFS/default/500_selinux_autorelabel.sh
Source restore/default/500_selinux_autorelabel.sh
Source restore/NETFS/Linux-i386/510_selinux_fixfiles_exclude_dirs.sh
so I wonder it it wouldn't be simpler to have that code only in usr/share/rear/restore/default/500_selinux_autorelabel.sh (perhaps with some additional conditions to run it there)?

Hello,

Yes, I added it in both files because I was looking for the .autorelabel file, where this should be executed.
But I did'nt dig to see why it's in both places. Of course, it will be better to have only one.

Thanks

pcahyna commented at 2021-11-23 14:19:¶

@jsmeix I believe that the whole SELinux handling needs some overhaul (I saw some oddities when working on the rsync cleanup, but I had no time to dig deeper).
@bwelterl why is it needed at all for BACKUP=NETFS? I thought that you are concerned about BACKUP=TSM and BACKUP=RSYNC.
I suspect that for tar and rsync the whole relabeling stuff might be unneeded nowadays and it should be kept only for methods like TSM.

jsmeix commented at 2021-11-23 14:39:¶

@pcahyna
beware of digging too deep!
You may wake up demons lurking in the dark, cf.
https://en.wikipedia.org/wiki/Moria_(Middle-earth)

The Dwarves dug too deep, greedy for mithril,
and disturbed a demon of great power:
a Balrog, which destroyed their kingdom

jsmeix commented at 2021-11-24 10:01:¶

@pcahyna
regarding what you wrote above in your
https://github.com/rear/rear/pull/2717#issuecomment-976603335

I suspect that for tar and rsync the whole relabeling stuff might be
unneeded nowadays and it should be kept only for methods like TSM

I wonder if such "relabeling stuff" should depend on the backup method
because I think this would be RFC 1925 item 6a
"It is always possible to add another level of indirection."
https://datatracker.ietf.org/doc/html/rfc1925

When the condition that is tested to find out whether or not "relabeling stuff"
needs to be done is the backup method then not the actual condition is tested
but some indirect condition that may mostly work but will sometimes fail, e.g.
think about BACKUP=REQUESTRESTORE where the user uses TSM
(for whatever reason he wants to restore manually).

So I think there should be a generic script that is always run after the backup was restored
that tests an actual condition to find out whether or not "relabeling stuff" needs to be done.

Such an actual test could be something like the above in
https://github.com/rear/rear/pull/2717#issuecomment-976382378
mentioned getfattr test for the restored usr/bin/ping
i.e. /mnt/local/usr/bin/ping or via chroot inside the restored system.

To be more fail save and not depend on particular restored files of the system
(the user may have removed extended attributes from his /usr/bin/ping)
ReaR may create its own file with extended attributes in an appropriate directory
(i.e. a directory that is likely included in the backup so ReaR's file gets restored,
my first offhanded idea is CONFIG_DIR="$REAR_DIR_PREFIX/etc/rear")
and test that one (if that file is not restored do some reasonable fallback).

bwelterl commented at 2021-11-24 14:12:¶

Concerning extended attributes in general and specifically in rsync, see #2577 (comment) :
Not that the rsync code is entirely correct: it does not restore extended attributes: so before restore you have
$ getfattr -d -m- /usr/bin/ping
getfattr: Removing leading '/' from absolute path names
# file: usr/bin/ping
security.capability=0sAAAAAgAwAAAAAAAAAAAAAAAAAAA=
security.selinux="system_u:object_r:ping_exec_t:s0"
while after restore just
getfattr -d -m- /usr/bin/ping
getfattr: Removing leading '/' from absolute path names
# file: usr/bin/ping
security.selinux="system_u:object_r:ping_exec_t:s0"
so the system will be a bit broken in different ways.
So, while your change makes sense for TSM, I suspect that it is only papering over a deeper problem for RSYNC. I will look at solving the problem in more generic ways for RSYNC. In the meantime, can you please try the getfattr test above for the restored systems, especially with TSM, as I don't have access to TSM? I would like to know whether the TSM problems are limited to SELinux attributes.

Hello,

The output after the after the restore with TSM is the expected one:

# getfattr -d -m- /usr/bin/ping
getfattr: Removing leading '/' from absolute path names

# file: usr/bin/ping
security.capability=0sAAAAAgAwAAAAAAAAAAAAAAAAAAA=
security.selinux="system_u:object_r:ping_exec_t:s0"

But this is for a file, where the selinux xattr are not affected with TSM. It's only links that lose the xattrs.
I'm not aware of a link with extended attributes.
Do you have one from the system ?
Tx

github-actions commented at 2022-03-05 02:23:¶

Stale pull request message

github-actions commented at 2022-06-07 03:09:¶

Stale pull request message

jsmeix commented at 2022-06-09 08:39:¶

@pcahyna
can this issue be solved for ReaR 2.7
or should it be postponed to ReaR 2.8?

pcahyna commented at 2022-06-09 09:14:¶

@jsmeix I would leave it for ReaR 2.8. It is not a regression, and the particular regression in systemd that triggered the discussion has been fixed, see https://github.com/rear/rear/issues/2716#issuecomment-1102931847 , lowering the priority.

jsmeix commented at 2022-06-09 10:30:¶

I postponed this issue to ReaR 2.8

github-actions commented at 2022-08-09 03:26:¶

Stale pull request message

[Export of Github issue for rear/rear.]

#2717 PR closed: Add a prerelabel with setfiles before the first reboot, because autorelabel is not always reliable¶

bwelterl opened issue at 2021-11-19 14:26:¶

Relax-and-Recover (ReaR) Pull Request Template¶

Pull Request Details:¶

pcahyna commented at 2021-11-19 14:57:¶

bwelterl commented at 2021-11-19 15:07:¶

bwelterl commented at 2021-11-22 08:56:¶

hpannenb commented at 2021-11-22 18:10:¶

pcahyna commented at 2021-11-23 10:33:¶

jsmeix commented at 2021-11-23 14:08:¶

bwelterl commented at 2021-11-23 14:11:¶

pcahyna commented at 2021-11-23 14:19:¶

jsmeix commented at 2021-11-23 14:39:¶

jsmeix commented at 2021-11-24 10:01:¶

bwelterl commented at 2021-11-24 14:12:¶

github-actions commented at 2022-03-05 02:23:¶

github-actions commented at 2022-06-07 03:09:¶

jsmeix commented at 2022-06-09 08:39:¶

pcahyna commented at 2022-06-09 09:14:¶

jsmeix commented at 2022-06-09 10:30:¶

github-actions commented at 2022-08-09 03:26:¶

#2717 PR `closed`: Add a prerelabel with setfiles before the first reboot, because autorelabel is not always reliable¶