Merge pull request #2255 from DaanDeMeyer/simplify

Simplify apivfs_cmd() and chroot_cmd()

mkosi/__init__.py

@@ -43,6 +43,7 @@
     format_tree,
     parse_config,
     summary,
+    want_selinux_relabel,
     yes_no,
 )
 from mkosi.context import Context
@@ -442,7 +443,6 @@ def run_prepare_scripts(context: Context, build: bool) -> None:
             chroot = chroot_cmd(
                 context.root,
                 resolve=True,
-                tools=context.config.tools(),
                 options=[
                     "--bind", "/work", "/work",
                     "--chdir", "/work/src",
@@ -517,7 +517,6 @@ def run_build_scripts(context: Context) -> None:
             chroot = chroot_cmd(
                 context.root,
                 resolve=context.config.with_network,
-                tools=context.config.tools(),
                 options=[
                     "--bind", "/work", "/work",
                     "--chdir", "/work/src",
@@ -587,7 +586,6 @@ def run_postinst_scripts(context: Context) -> None:
             chroot = chroot_cmd(
                 context.root,
                 resolve=context.config.with_network,
-                tools=context.config.tools(),
                 options=[
                     "--bind", "/work", "/work",
                     "--chdir", "/work/src",
@@ -648,7 +646,6 @@ def run_finalize_scripts(context: Context) -> None:
             chroot = chroot_cmd(
                 context.root,
                 resolve=context.config.with_network,
-                tools=context.config.tools(),
                 options=[
                     "--bind", "/work", "/work",
                     "--chdir", "/work/src",
@@ -2358,25 +2355,7 @@ def run_firstboot(context: Context) -> None:
 
 
 def run_selinux_relabel(context: Context) -> None:
-    if context.config.selinux_relabel == ConfigFeature.disabled:
-        return
-
-    selinux = context.root / "etc/selinux/config"
-    if not selinux.exists():
-        if context.config.selinux_relabel == ConfigFeature.enabled:
-            die("SELinux relabel is requested but could not find selinux config at /etc/selinux/config")
-        return
-
-    policy = run(["sh", "-c", f". {selinux} && echo $SELINUXTYPE"],
-                 sandbox=context.sandbox(options=["--ro-bind", selinux, selinux]),
-                 stdout=subprocess.PIPE).stdout.strip()
-    if not policy:
-        if context.config.selinux_relabel == ConfigFeature.enabled:
-            die("SELinux relabel is requested but no selinux policy is configured in /etc/selinux/config")
-        return
-
-    if not find_binary("setfiles", root=context.config.tools()):
-        logging.info("setfiles is not installed, not relabeling files")
+    if not (policy := want_selinux_relabel(context.config, context.root)):
         return
 
     fc = context.root / "etc/selinux" / policy / "contexts/files/file_contexts"

mkosi/config.py

@@ -3561,3 +3561,29 @@ def json_transformer(key: str, val: Any) -> Any:
         return val
 
     return json_transformer
+
+
+def want_selinux_relabel(config: Config, root: Path, fatal: bool = True) -> Optional[str]:
+    if config.selinux_relabel == ConfigFeature.disabled:
+        return None
+
+    selinux = root / "etc/selinux/config"
+    if not selinux.exists():
+        if fatal and config.selinux_relabel == ConfigFeature.enabled:
+            die("SELinux relabel is requested but could not find selinux config at /etc/selinux/config")
+        return None
+
+    policy = run(["sh", "-c", f". {selinux} && echo $SELINUXTYPE"],
+                 sandbox=config.sandbox(options=["--ro-bind", selinux, selinux]),
+                 stdout=subprocess.PIPE).stdout.strip()
+    if not policy:
+        if fatal and config.selinux_relabel == ConfigFeature.enabled:
+            die("SELinux relabel is requested but no selinux policy is configured in /etc/selinux/config")
+        return None
+
+    if not find_binary("setfiles", root=config.tools()):
+        if fatal:
+            logging.info("setfiles is not installed, not relabeling files")
+        return None
+
+    return policy

mkosi/distributions/gentoo.py

@@ -26,7 +26,7 @@
 
 def invoke_emerge(context: Context, packages: Sequence[str] = (), apivfs: bool = True) -> None:
     run(
-        apivfs_cmd(context.root, tools=context.config.tools()) + [
+        apivfs_cmd(context.root) + [
             # We can't mount the stage 3 /usr using `options`, because bwrap isn't available in the stage 3
             # tarball which is required by apivfs_cmd(), so we have to mount /usr from the tarball later
             # using another bwrap exec.
@@ -161,7 +161,6 @@ def install(cls, context: Context) -> None:
 
         chroot = chroot_cmd(
             stage3,
-            tools=context.config.tools(),
             options=["--bind", context.cache_dir / "repos", "/var/db/repos"],
         )
 

mkosi/installer/__init__.py

@@ -41,12 +41,12 @@ def clean_package_manager_metadata(context: Context) -> None:
 
 def package_manager_scripts(context: Context) -> dict[str, list[PathString]]:
     return {
-        "pacman": apivfs_cmd(context.root, tools=context.config.tools()) + pacman_cmd(context),
-        "zypper": apivfs_cmd(context.root, tools=context.config.tools()) + zypper_cmd(context),
-        "dnf"   : apivfs_cmd(context.root, tools=context.config.tools()) + dnf_cmd(context),
-        "rpm"   : apivfs_cmd(context.root, tools=context.config.tools()) + rpm_cmd(context),
+        "pacman": apivfs_cmd(context.root) + pacman_cmd(context),
+        "zypper": apivfs_cmd(context.root) + zypper_cmd(context),
+        "dnf"   : apivfs_cmd(context.root) + dnf_cmd(context),
+        "rpm"   : apivfs_cmd(context.root) + rpm_cmd(context),
     } | {
-        command: apivfs_cmd(context.root, tools=context.config.tools()) + apt_cmd(context, command) for command in (
+        command: apivfs_cmd(context.root) + apt_cmd(context, command) for command in (
             "apt",
             "apt-cache",
             "apt-cdrom",

mkosi/installer/apt.py

@@ -113,7 +113,7 @@ def invoke_apt(
                     *finalize_crypto_mounts(tools=context.config.tools()),
                     *mounts,
                 ],
-            ) + (apivfs_cmd(context.root, tools=context.config.tools()) if apivfs else [])
+            ) + (apivfs_cmd(context.root) if apivfs else [])
         ),
         env=context.config.environment,
     )

mkosi/installer/dnf.py

@@ -139,7 +139,7 @@ def invoke_dnf(context: Context, command: str, packages: Iterable[str], apivfs:
                     context.cache_dir / "lib" / dnf_subdir(context),
                     *finalize_crypto_mounts(tools=context.config.tools()),
                 ],
-            ) + (apivfs_cmd(context.root, tools=context.config.tools()) if apivfs else [])
+            ) + (apivfs_cmd(context.root) if apivfs else [])
         ),
         env=context.config.environment,
     )

mkosi/installer/pacman.py

@@ -99,7 +99,7 @@ def invoke_pacman(
                     "--bind", context.cache_dir / "cache/pacman/pkg", context.cache_dir / "cache/pacman/pkg",
                     *finalize_crypto_mounts(tools=context.config.tools()),
                 ],
-            ) + (apivfs_cmd(context.root, tools=context.config.tools()) if apivfs else [])
+            ) + (apivfs_cmd(context.root) if apivfs else [])
         ),
         env=context.config.environment,
     )

mkosi/installer/zypper.py

@@ -87,7 +87,7 @@ def invoke_zypper(
                     "--bind", context.cache_dir / "cache/zypp", context.cache_dir / "cache/zypp",
                     *finalize_crypto_mounts(tools=context.config.tools()),
                 ],
-            ) + (apivfs_cmd(context.root, tools=context.config.tools()) if apivfs else [])
+            ) + (apivfs_cmd(context.root) if apivfs else [])
         ),
         env=context.config.environment,
     )

mkosi/qemu.py

@@ -30,6 +30,7 @@
     QemuFirmware,
     QemuVsockCID,
     format_bytes,
+    want_selinux_relabel,
 )
 from mkosi.log import die
 from mkosi.partition import finalize_root, find_partitions
@@ -326,7 +327,7 @@ def start_virtiofsd(config: Config, directory: Path, *, uidmap: bool) -> Iterato
         "--sandbox=chroot",
     ]
 
-    if not uidmap:
+    if not uidmap and want_selinux_relabel(config, directory, fatal=False):
         cmdline += ["--security-label"]
 
     # We create the socket ourselves and pass the fd to virtiofsd to avoid race conditions where we start qemu

mkosi/run.py

@@ -289,6 +289,12 @@ def preexec() -> None:
         if preexec_fn:
             preexec_fn()
 
+    if (
+        sandbox and
+        subprocess.run(sandbox + ["sh", "-c", "command -v setpgid"], stdout=subprocess.DEVNULL).returncode == 0
+    ):
+        cmdline = ["setpgid", "--foreground", "--"] + cmdline
+
     try:
         # subprocess.run() will use SIGKILL to kill processes when an exception is raised.
         # We'd prefer it to use SIGTERM instead but since this we can't configure which signal
@@ -373,6 +379,13 @@ def preexec() -> None:
         if preexec_fn:
             preexec_fn()
 
+    if (
+        foreground and
+        sandbox and
+        subprocess.run(sandbox + ["sh", "-c", "command -v setpgid"], stdout=subprocess.DEVNULL).returncode == 0
+    ):
+        cmdline = ["setpgid", "--foreground", "--"] + cmdline
+
     try:
         with subprocess.Popen(
             sandbox + cmdline,

mkosi/sandbox.py

@@ -7,7 +7,6 @@
 from pathlib import Path
 from typing import Optional
 
-from mkosi.run import find_binary
 from mkosi.types import PathString
 from mkosi.util import INVOKING_USER, flatten, one_zero
 
@@ -33,18 +32,12 @@ def finalize_passwd_mounts(root: Path) -> list[PathString]:
     """
     If passwd or a related file exists in the apivfs directory, bind mount it over the host files while we
     run the command, to make sure that the command we run uses user/group information from the apivfs
-    directory instead of from the host. If the file doesn't exist yet, mount over /dev/null instead.
+    directory instead of from the host.
     """
     options: list[PathString] = []
 
     for f in ("passwd", "group", "shadow", "gshadow"):
-        if not (Path("/etc") / f).exists():
-            continue
-        p = root / "etc" / f
-        if p.exists():
-            options += ["--bind", p, f"/etc/{f}"]
-        else:
-            options += ["--bind", "/dev/null", f"/etc/{f}"]
+        options += ["--ro-bind-try", root / "etc" / f, f"/etc/{f}"]
 
     return options
 
@@ -102,7 +95,10 @@ def sandbox_cmd(
     if relaxed:
         cmdline += ["--bind", "/tmp", "/tmp"]
     else:
-        cmdline += ["--tmpfs", "/tmp"]
+        cmdline += [
+            "--tmpfs", "/tmp",
+            "--unshare-ipc",
+        ]
 
     if (tools / "nix/store").exists():
         cmdline += ["--bind", tools / "nix/store", "/nix/store"]
@@ -161,14 +157,11 @@ def sandbox_cmd(
 
     cmdline += ["sh", "-c", f"{shm} && exec $0 \"$@\""]
 
-    if setpgid := find_binary("setpgid", root=tools):
-        cmdline += [setpgid, "--foreground", "--"]
-
     return cmdline
 
 
-def apivfs_cmd(root: Path, *, tools: Path = Path("/")) -> list[PathString]:
-    cmdline: list[PathString] = [
+def apivfs_cmd(root: Path) -> list[PathString]:
+    return [
         "bwrap",
         "--dev-bind", "/", "/",
         "--tmpfs", root / "run",
@@ -178,38 +171,30 @@ def apivfs_cmd(root: Path, *, tools: Path = Path("/")) -> list[PathString]:
         "--dev", root / "dev",
         # APIVFS generally means chrooting is going to happen so unset TMPDIR just to be safe.
         "--unsetenv", "TMPDIR",
-    ]
-
-    if (root / "etc/machine-id").exists():
         # Make sure /etc/machine-id is not overwritten by any package manager post install scripts.
-        cmdline += ["--ro-bind", root / "etc/machine-id", root / "etc/machine-id"]
-
-    cmdline += finalize_passwd_mounts(root)
-
-    if setpgid := find_binary("setpgid", root=tools):
-        cmdline += [setpgid, "--foreground", "--"]
-
-    chmod = f"chmod 1777 {root / 'tmp'} {root / 'var/tmp'} {root / 'dev/shm'}"
-    # Make sure anything running in the root directory thinks it's in a container. $container can't always be
-    # accessed so we write /run/host/container-manager as well which is always accessible.
-    container = f"mkdir {root}/run/host && echo mkosi >{root}/run/host/container-manager"
-
-    cmdline += ["sh", "-c", f"{chmod} && {container} && exec $0 \"$@\""]
-
-    return cmdline
+        "--ro-bind-try", root / "etc/machine-id", root / "etc/machine-id",
+        *finalize_passwd_mounts(root),
+        "sh", "-c",
+        f"chmod 1777 {root / 'tmp'} {root / 'var/tmp'} {root / 'dev/shm'} && "
+        f"chmod 755 {root / 'run'} && "
+        # Make sure anything running in the root directory thinks it's in a container. $container can't always be
+        # accessed so we write /run/host/container-manager as well which is always accessible.
+        f"mkdir -m 755 {root}/run/host && echo mkosi >{root}/run/host/container-manager && "
+        "exec $0 \"$@\"",
+    ]
 
 
-def chroot_cmd(
-    root: Path,
-    *,
-    resolve: bool = False,
-    tools: Path = Path("/"),
-    options: Sequence[PathString] = (),
-) -> list[PathString]:
+def chroot_cmd(root: Path, *, resolve: bool = False, options: Sequence[PathString] = ()) -> list[PathString]:
     cmdline: list[PathString] = [
         "sh", "-c",
+        f"trap 'rm -rf {root / 'work'}' EXIT && "
+        # /etc/resolv.conf can be a dangling symlink to /run/systemd/resolve/stub-resolv.conf. Bubblewrap tries to call
+        # mkdir() on each component of the path which means it will try to call
+        # mkdir(/run/systemd/resolve/stub-resolv.conf) which will fail unless /run/systemd/resolve exists already so
+        # we make sure that it already exists.
+        f"mkdir -p -m 755 {root / 'work'} {root / 'run/systemd'} {root / 'run/systemd/resolve'} && "
         # No exec here because we need to clean up the /work directory afterwards.
-        f"trap 'rm -rf {root / 'work'}' EXIT && mkdir -p {root / 'work'} && chown 777 {root / 'work'} && $0 \"$@\"",
+        f"$0 \"$@\"",
         "bwrap",
         "--dev-bind", root, "/",
         "--setenv", "container", "mkosi",
@@ -218,20 +203,8 @@ def chroot_cmd(
     ]
 
     if resolve:
-        p = Path("etc/resolv.conf")
-        if (root / p).is_symlink():
-            # For each component in the target path, bubblewrap will try to create it if it doesn't exist
-            # yet. If a component in the path is a dangling symlink, bubblewrap will end up calling
-            # mkdir(symlink) which obviously fails if multiple components of the dangling symlink path don't
-            # exist yet. As a workaround, we resolve the symlink ourselves so that bubblewrap will correctly
-            # create all missing components in the target path.
-            p = p.parent / (root / p).readlink()
-
-        cmdline += ["--ro-bind", "/etc/resolv.conf", Path("/") / p]
-
-    cmdline += [*options]
+        cmdline += ["--ro-bind-try", "/etc/resolv.conf", "/etc/resolv.conf"]
 
-    if setpgid := find_binary("setpgid", root=root):
-        cmdline += [setpgid, "--foreground", "--"]
+    cmdline += options
 
-    return apivfs_cmd(root, tools=tools) + cmdline
+    return apivfs_cmd(root) + cmdline