From e8adfc422fc2891d33eb9aa4848bb4afa7bc9c0a Mon Sep 17 00:00:00 2001 From: Daan De Meyer Date: Mon, 8 Jan 2024 23:31:37 +0100 Subject: [PATCH 1/3] Simplify apivfs_cmd() and chroot_cmd() We move the setpgid logic to run(), avoiding the need to pass a tools argument to chroot_cmd() and apivfs_cmd(). We also try to remove as much logic from these functions as possible. Since we can't really assume that any logic we execute during the function will still hold true in the sandbox, so it's best to delay any logic execution until we're already in the sandbox (using the --ro-bind-try options of bubblewrap). We also rework the /etc/resolv.conf handling to simply make sure that /run/systemd/resolve exists in the chroot since if /etc/resolv.conf points to /run it'll almost certainly be to /run/systemd/resolv/stub-resolv.conf. --- mkosi/__init__.py | 4 -- mkosi/distributions/gentoo.py | 3 +- mkosi/installer/__init__.py | 10 ++--- mkosi/installer/apt.py | 2 +- mkosi/installer/dnf.py | 2 +- mkosi/installer/pacman.py | 2 +- mkosi/installer/zypper.py | 2 +- mkosi/run.py | 13 ++++++ mkosi/sandbox.py | 80 +++++++++++------------------------ 9 files changed, 48 insertions(+), 70 deletions(-) diff --git a/mkosi/__init__.py b/mkosi/__init__.py index 66a138ea4..924dc9f48 100644 --- a/mkosi/__init__.py +++ b/mkosi/__init__.py @@ -442,7 +442,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 +516,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 +585,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 +645,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", diff --git a/mkosi/distributions/gentoo.py b/mkosi/distributions/gentoo.py index 1dbff224f..e6dfe95bc 100644 --- a/mkosi/distributions/gentoo.py +++ b/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"], ) diff --git a/mkosi/installer/__init__.py b/mkosi/installer/__init__.py index 36afdab73..6135e03e0 100644 --- a/mkosi/installer/__init__.py +++ b/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", diff --git a/mkosi/installer/apt.py b/mkosi/installer/apt.py index 594bdb6b0..0ccecebf0 100644 --- a/mkosi/installer/apt.py +++ b/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, ) diff --git a/mkosi/installer/dnf.py b/mkosi/installer/dnf.py index c410c7141..8a7c283f6 100644 --- a/mkosi/installer/dnf.py +++ b/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, ) diff --git a/mkosi/installer/pacman.py b/mkosi/installer/pacman.py index 1c3cfd905..993aeaebf 100644 --- a/mkosi/installer/pacman.py +++ b/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, ) diff --git a/mkosi/installer/zypper.py b/mkosi/installer/zypper.py index c800038f9..91e0caadd 100644 --- a/mkosi/installer/zypper.py +++ b/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, ) diff --git a/mkosi/run.py b/mkosi/run.py index 54885d3f3..5fd7ef12f 100644 --- a/mkosi/run.py +++ b/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, diff --git a/mkosi/sandbox.py b/mkosi/sandbox.py index 1e730fdae..e574fd4b9 100644 --- a/mkosi/sandbox.py +++ b/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 @@ -161,14 +154,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 +168,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 +200,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 From 0ffba6daec10334787beb5de78f5993d2df62baa Mon Sep 17 00:00:00 2001 From: Daan De Meyer Date: Tue, 9 Jan 2024 08:49:00 +0100 Subject: [PATCH 2/3] Only set --security-label if the filesystem was relabeled Otherwise we run into virtiofsd errors when operating on non relabeled directories with --security-label enabled. --- mkosi/__init__.py | 21 ++------------------- mkosi/config.py | 26 ++++++++++++++++++++++++++ mkosi/qemu.py | 3 ++- 3 files changed, 30 insertions(+), 20 deletions(-) diff --git a/mkosi/__init__.py b/mkosi/__init__.py index 924dc9f48..ab42a1876 100644 --- a/mkosi/__init__.py +++ b/mkosi/__init__.py @@ -43,6 +43,7 @@ format_tree, parse_config, summary, + want_selinux_relabel, yes_no, ) from mkosi.context import Context @@ -2354,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" diff --git a/mkosi/config.py b/mkosi/config.py index 0b57548e8..8209a58ef 100644 --- a/mkosi/config.py +++ b/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 diff --git a/mkosi/qemu.py b/mkosi/qemu.py index 267df7f22..fa3d518f1 100644 --- a/mkosi/qemu.py +++ b/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 From 2227eb2411fb5624e6981a06f18f3225fe1a41eb Mon Sep 17 00:00:00 2001 From: Daan De Meyer Date: Tue, 9 Jan 2024 10:26:03 +0100 Subject: [PATCH 3/3] Unshare IPC namespace when not in relaxed mode Otherwise tests in the sandbox will think they have access to IPC stuff when they actually don't. Fixes #2256 --- mkosi/sandbox.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/mkosi/sandbox.py b/mkosi/sandbox.py index e574fd4b9..0e1921d02 100644 --- a/mkosi/sandbox.py +++ b/mkosi/sandbox.py @@ -95,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"]