Repository: osbuild/osbuild
Pull request #1737 (2024-04-16 10:47:32)

Stages/SELinux: force auto-relabel of full contexts
Previously, the SELinux stage would not force full contexts reset when forcing auto-relabel on first boot. As a result, all files remained `unconfined_u` after the auto-relabeling on first boot and only the type part was reset.

We really need to mimic the behavior of `fixfiles -F onboot` command, which creates the `/.autorelabel` file with "-F" in it.

achilleas-k: Two thoughts:
1. wow
2. Is this a behaviour change that we should encode in a stage option?
thozza: > Two thoughts:
> 
> 1. wow
:smiling_face_with_tear: 
> 2. Is this a behaviour change that we should encode in a stage option?

Technically it is a behavior change in the meaning that previously the forced relabeling was not really forcing anything and it was not doing what was IMO expected. We always **force** the relabeling when not doing it on first boot by passing `-F` https://github.com/thozza/osbuild/blob/b5b3a5ba8c5f665e960548b6a69d48c09b5e11f0/osbuild/util/selinux.py#L48.

I think that the not forced relabeling is not actually useful at all. For these reasons, I do not think that it makes sense to add a new stage option for this (it would be something like `really_force_autorelabel` :innocent: ).

mvo5: [..]
> > 2. Is this a behaviour change that we should encode in a stage option?
> 
> Technically it is a behavior change in the meaning that previously the forced relabeling was not really forcing anything and it was not doing what was IMO expected. We always **force** the relabeling when not doing it on first boot by passing `-F` https://github.com/thozza/osbuild/blob/b5b3a5ba8c5f665e960548b6a69d48c09b5e11f0/osbuild/util/selinux.py#L48.
> 
> I think that the not forced relabeling is not actually useful at all. For these reasons, I do not think that it makes sense to add a new stage option for this (it would be something like `really_force_autorelabel` 😇 ).

Thanks for finding this! I can only echo the "woah"!

How did find this? And where/how is autorelabel used? Given that it seems to be broken for a long time I wonder why we haven't noticed before (and how we could functionally test for it)?
mvo5: We should probably also file a bug against the selinux man-page, as in https://github.com/SELinuxProject/selinux/blob/main/policycoreutils/man/man5/selinux_config.5#L114 this behavior is not explained at all (and neither https://github.com/SELinuxProject/selinux/blob/main/libselinux/man/man8/selinux.8#L90).
thozza: The `/.autorelabel` file gets created by `fixfiles [-F] onboot` command. There is https://access.redhat.com/solutions/24845 which suggests touching the file or calling `fixfiles`, however it does not mention `-F`.
achilleas-k: Rebasing on `main` because of the gh workflow file changes.
thozza: I had to fix pylint issue (and added comment about adding `-F` to the `.autorelabel` file.
thozza: Marking as Draft until I figure out the weird unit test failures... :shrug: 
thozza: OK, I'm out of ideas... I can't reproduce this locally (not even in the CI container). Modifying `stages/org.osbuild.systemd` file makes the unit tests pass... :confused: 
achilleas-k: :exploding_head: 
supakeen: Excellent debugging by @mvo5 on this. Since we do some slightly funky importing of stages in the tests they all end up under the same import name which ends up with clashing bytecode cache names.

We don't use the same process in osbuild itself.

Turning off the bytecode by passing `-B` in `tox.ini` to `python` or setting `sys.dont_write_bytecode = True` in `osbuild/testutil/imports.py` will work around this.
mvo5: > Excellent debugging by @mvo5 on this. Since we do some slightly funky importing of stages in the tests they all end up under the same import name which ends up with clashing bytecode cache names.
> 
> We don't use the same process in osbuild itself.
> 
> Turning off the bytecode by passing `-B` in `tox.ini` to `python` or setting `sys.dont_write_bytecode = True` in `osbuild/testutil/imports.py` will work around this.

Funky importing (i.e. using SourceFileLoader) and funky filenames for our stages :) (i.e. the most significant part of a stage is what for most systems is the extension and that is cut away by the pycache system, i.e. `org.osbuild.systemd -> org.osbuild.cpython-311.py`) gets the extension split off and it's assumes the basename is the relevant part (c.f. https://github.com/python/cpython/blob/main/Lib/importlib/_bootstrap_external.py#L534).

Patch:
diff --git a/stages/org.osbuild.selinux b/stages/org.osbuild.selinux
index cf5e850a..bb45298d 100755
--- a/stages/org.osbuild.selinux
+++ b/stages/org.osbuild.selinux
@@ -21,7 +21,13 @@ def main(tree, options):
 
     if options.get("force_autorelabel", False):
         stamp = pathlib.Path(tree, ".autorelabel")
-        stamp.touch()
+        # Creating just empty /.autorelabel resets only the type of files.
+        # To ensure that the full context is reset, we write "-F" into the file.
+        # This mimics the behavior of `fixfiles -F boot`. The "-F" option is
+        # then passed to `selinux-autorelabel` script [0].
+        # Note that this is missing from the selinux(8) and selinux_config(5) man-pages
+        # [0] https://src.fedoraproject.org/rpms/policycoreutils/blob/rawhide/f/selinux-autorelabel#_54
+        stamp.write_text("-F", encoding="utf-8")
 
 
 if __name__ == '__main__':


Test patch:
diff --git a/osbuild/testutil/imports.py b/osbuild/testutil/imports.py
index 3fb14aee..12969282 100644
--- a/osbuild/testutil/imports.py
+++ b/osbuild/testutil/imports.py
@@ -3,8 +3,16 @@
 Import related utilities
 """
 import importlib
+import sys
 from types import ModuleType
 
+# Cache files will split the extension, this means that all pyc cache files
+# looks like we get many clashing `org.osbuild.cpython-py311.pyc` files.
+# Moreover, the cache bytecode invalidation is based on the timestamp (which
+# is the same after git checkout) and the file size (which may be the same
+# for two different files). This means that we can't rely on the cache files.
+sys.dont_write_bytecode = True
+
 
 def import_module_from_path(fullname, path: str) -> ModuleType:
     """import_module_from_path imports the given path as a python module
diff --git a/stages/test/test_selinux.py b/stages/test/test_selinux.py
index 27eba4a5..dca271b6 100644
--- a/stages/test/test_selinux.py
+++ b/stages/test/test_selinux.py
@@ -114,3 +114,5 @@ def test_selinux_force_autorelabel(mocked_setfiles, tmp_path, stage_module):  #
         stage_module.main(tmp_path, options)
 
         assert (tmp_path / ".autorelabel").exists() == enable_autorelabel
+        if enable_autorelabel:
+            assert (tmp_path / ".autorelabel").read_text() == "-F"