Repository: osbuild/osbuild
Pull request #993 (2022-03-17 15:13:48)

stages/selinux: directly call setfilecon
Instead of using `chcon`, directly call `selinux.setfilecon`. On systems without SELinux support, i.e. coreutils was built without `<selinux.h>` present, `chcon` will return `ENOTSUP` for all selinux related calls like `setfilecon` even if the selinux libraries are later installed. Therefore we directly call the library function which should ensure that we don't error out as long as the library is present.  The only other thing `chcon` is doing besides a cal to the `setfilecon` method is to convert the context string to a `contex_t` and back to validate it. This should not be needed since the kernel will do this for us. On system without SELinux support `context_new` will also not validate the context.

jelly: I tried re-running the LGTM CI, but Github is really confused so I think you have to re-force push :confused: @gicmo  
gicmo: Apparently we are missing `python3-libselinux` on the CI image.
gicmo: I checked our 8.x images and we started labelling `cp` in 8.5 and I think in that images we have `python3-libselinux` in the build root everywhere; but since this technically a (breaking) behaviour change, I will change this to _try_ to use `selinux.setfilecon` and if that doesn't work fall back to `chcon`.
gicmo: On a second though, `setfilecon` is such a [simple wrapper](https://github.com/SELinuxProject/selinux/blob/d155b410d4bbc90d28f361b966f0429598da8188/libselinux/src/setfilecon.c#L10) around extended attributes, maybe I just reimplement that in our `utils.selinux: 


achilleas-k: > On a second though, `setfilecon` is such a [simple wrapper](https://github.com/SELinuxProject/selinux/blob/d155b410d4bbc90d28f361b966f0429598da8188/libselinux/src/setfilecon.c#L10) around extended attributes, maybe I just reimplement that in our `utils.selinux:

oh neat!
gicmo: Also locally verified that this is still doing the correct thing:
```
gicmo@snow ~/C/o/osbuild → ls -laZ /var/osbuild/store/refs/9590527d97c7cffe87595ff5a239b605258610e2cad0df8a7979e4f9c83d7e5b/bin/cp                                                                  
-rwxr-xr-x. 1 root root system_u:object_r:install_exec_t:s0 149776 Mar 26  2021 /var/osbuild/store/refs/9590527d97c7cffe87595ff5a239b605258610e2cad0df8a7979e4f9c83d7e5b/bin/cp
```

Patch:
diff --git a/osbuild/util/selinux.py b/osbuild/util/selinux.py
index 26ad5995..46c1e6a2 100644
--- a/osbuild/util/selinux.py
+++ b/osbuild/util/selinux.py
@@ -1,10 +1,14 @@
 """SELinux utility functions"""
 
+import errno
 import os
 import subprocess
 
 from typing import Dict, TextIO
 
+# Extended attribute name for SELinux labels
+XATTR_NAME_SELINUX = b"security.selinux"
+
 
 def parse_config(config_file: TextIO):
     """Parse an SELinux configuration file"""
@@ -51,6 +55,30 @@ def setfiles(spec_file: str, root: str, *paths):
 
 def getfilecon(path: str) -> str:
     """Get the security context associated with `path`"""
-    label = os.getxattr(path, b"security.selinux",
+    label = os.getxattr(path, XATTR_NAME_SELINUX,
                         follow_symlinks=False)
     return label.decode().strip('\n\0')
+
+
+def setfilecon(path: str, context: str) -> None:
+    """
+    Set the security context associated with `path`
+
+    Like `setfilecon`(3), but does not attempt to translate
+    the context via `selinux_trans_to_raw_context`.
+    """
+
+    try:
+        os.setxattr(path, XATTR_NAME_SELINUX,
+                    context.encode(),
+                    follow_symlinks=True)
+    except OSError as err:
+        # in case we get a not-supported error, check if
+        # the context we want to set is already set and
+        # ignore the error in that case. This follows the
+        # behavior of `setfilecon(3)`.
+        if err.errno == errno.ENOTSUP:
+            have = getfilecon(path)
+            if have == context:
+                return
+        raise
diff --git a/stages/org.osbuild.selinux b/stages/org.osbuild.selinux
index 2ca277e7..b7e0ffea 100755
--- a/stages/org.osbuild.selinux
+++ b/stages/org.osbuild.selinux
@@ -26,6 +26,7 @@ import subprocess
 import sys
 
 import osbuild.api
+from osbuild.util import selinux
 
 
 SCHEMA = """
@@ -60,7 +61,7 @@ def main(tree, options):
 
     for path, label in labels.items():
         fullpath = os.path.join(tree, path.lstrip("/"))
-        subprocess.run(["chcon", "-v", label, fullpath], check=True)
+        selinux.setfilecon(fullpath, label)
 
     if options.get("force_autorelabel", False):
         stamp = pathlib.Path(tree, ".autorelabel")


Test patch:
diff --git a/test/mod/test_util_selinux.py b/test/mod/test_util_selinux.py
index 74023723..c8524aa6 100644
--- a/test/mod/test_util_selinux.py
+++ b/test/mod/test_util_selinux.py
@@ -2,41 +2,59 @@
 # Tests for the 'osbuild.util.selinux' module.
 #
 
+import errno
 import io
-import unittest
+from unittest import mock
 
 from osbuild.util import selinux
 
 
-class TestObjectStore(unittest.TestCase):
-
-    def test_selinux_config(self):
-        f = io.StringIO()
-        cfg = selinux.parse_config(f)
-        self.assertIsNotNone(cfg)
-        policy = selinux.config_get_policy(cfg)
-        self.assertIsNone(policy)
-
-        example_good = """
-        # This file controls the state of SELinux on the system.
-        # SELINUX= can take one of these three values:
-        #     enforcing - SELinux security policy is enforced.
-        #     permissive - SELinux prints warnings instead of enforcing.
-        #     disabled - No SELinux policy is loaded.
-        SELINUX=enforcing
-        # SELINUXTYPE= can take one of these three values:
-        #     targeted - Targeted processes are protected,
-        #     minimum - Modification of targeted policy.
-        #     mls - Multi Level Security protection.
-        SELINUXTYPE=targeted
-        """
-
-        f = io.StringIO(example_good)
-        cfg = selinux.parse_config(f)
-        self.assertIn('SELINUX', cfg)
-        self.assertIn('SELINUXTYPE', cfg)
-        self.assertEqual(cfg['SELINUX'], 'enforcing')
-        self.assertEqual(cfg['SELINUXTYPE'], 'targeted')
-
-        policy = selinux.config_get_policy(cfg)
-        self.assertEqual(policy, 'targeted')
+def test_selinux_config():
+    f = io.StringIO()
+    cfg = selinux.parse_config(f)
+    assert cfg is not None
+    policy = selinux.config_get_policy(cfg)
+    assert policy is None
+
+    example_good = """
+    # This file controls the state of SELinux on the system.
+    # SELINUX= can take one of these three values:
+    #     enforcing - SELinux security policy is enforced.
+    #     permissive - SELinux prints warnings instead of enforcing.
+    #     disabled - No SELinux policy is loaded.
+    SELINUX=enforcing
+    # SELINUXTYPE= can take one of these three values:
+    #     targeted - Targeted processes are protected,
+    #     minimum - Modification of targeted policy.
+    #     mls - Multi Level Security protection.
+    SELINUXTYPE=targeted
+    """
+
+    f = io.StringIO(example_good)
+    cfg = selinux.parse_config(f)
+    assert 'SELINUX' in cfg
+    assert 'SELINUXTYPE' in cfg
+    assert cfg['SELINUX'] == 'enforcing'
+    assert cfg['SELINUXTYPE'] == 'targeted'
+
+    policy = selinux.config_get_policy(cfg)
+    assert policy == 'targeted'
+
+
+def test_setfilecon():
+    with mock.patch("os.setxattr") as setxattr:
+
+        selinux.setfilecon("/path", "context")
+        setxattr.assert_called_once_with("/path", selinux.XATTR_NAME_SELINUX,
+                                         b"context", follow_symlinks=True)
+
+    with mock.patch("os.getxattr") as getxattr:
+        with mock.patch("os.setxattr") as setxattr:
+
+            def raise_error(*_args, **_kwargs):
+                raise OSError(errno.ENOTSUP, "Not supported")
+
+            getxattr.return_value = b"context"
+            setxattr.side_effect = raise_error
+
+            selinux.setfilecon("path", "context")