diff --git a/CLAUDE.md b/CLAUDE.md index 20b0f2a..5f04d22 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -197,11 +197,28 @@ Source layout (all under `src/`): what `volume_mount.cpp`'s copy-into-an-empty-volume step uses to reach the image's content when running rootless (see below); `run_bwrap()` itself now just calls it once on the assembled `bwrap` argv. - `build_bwrap_args()` deliberately drops `--unshare-net` from what's actually passed - to `bwrap` even when the kernel supports it — without any network setup (e.g. - `slirp4netns`), unsharing it just leaves the sandbox with no network at all. Re-add - once network isolation is implemented; `detect_bwrap_unshare_args()` itself still - probes/reports it (e.g. via `-t/--test`), since that's kernel capability, not policy. + `build_bwrap_args()`/`run_bwrap()` also take a `NamespaceConfig` (`bwrap.h`) — one + plain `bool` field per `namespace_probes` entry (`user`/`ipc`/`pid`/`net`/`uts`/ + `cgroup`, default `true`), resolved by `run_container()` (`commands.cpp`) from + `AppConfig`'s six `global.unshare-*` keys (`config_file.h`, see above) once, up + front — `bwrap.{h,cpp}` itself never touches `AppConfig`/YAML, only this + already-resolved struct. For each flag `detect_bwrap_unshare_args()` finds the + kernel supports, `build_bwrap_args()` additionally requires the matching + `NamespaceConfig` field to be `true` (looked up via a `.cpp`-local + `namespace_policy_enabled()` if-chain over `namespace_probes`' `name`s) before + actually passing it to `bwrap` — kernel support and policy are separate gates, + both must allow a type. This replaced an earlier hardcoded special case that + always dropped `--unshare-net` regardless of policy or kernel support (without + any network setup, e.g. `slirp4netns`, unsharing it just left the sandbox with no + network at all) — `net` now goes through the exact same policy path as every + other type, defaulting to enabled like the rest. **This is a deliberate, + user-acknowledged transitional behavior change**: as of this, a plain `-r/--run` + with no config file override gets a real network namespace and thus no network + access at all, until `slirp4netns` integration (the next task on this same + branch) actually sets one up; `global.unshare-net: off` restores the prior + no-isolation behavior in the meantime. `detect_bwrap_unshare_args()` itself is + untouched by any of this — still an unfiltered kernel-capability probe, so + `-t/--test`'s diagnostic report continues to reflect raw kernel support, not policy. `build_bwrap_args()`/`run_bwrap()` also take an optional `hostname` (from `--hostname`, long-option only): passed through as bwrap's own `--hostname` only when `--unshare-uts` is actually among the flags `bwrap` is being given (bwrap @@ -666,25 +683,41 @@ Source layout (all under `src/`): - `config_file.{h,cpp}` — `load_config_file()` reads and parses (via libyaml's document API, ``) the `global` and `volumes` sections of the local YAML config file located by `config_file_path()` (`$XDG_CONFIG_HOME/slocker-lite/config.yaml`, - falling back to `$HOME/.config/slocker-lite/config.yaml`). `global.log-level` is the - only supported `global` key — other long options are one-shot flags, not settings, - so they don't belong in a persistent config file. A missing file returns a - default-constructed (empty) `AppConfig`, not an error; unknown sections/keys (and - malformed individual volume entries) are ignored for forward-compatibility; - malformed YAML syntax is a hard error. `main()` applies `config->log_level` (via - the existing `apply_log_level()`) right after `spdlog::cfg::load_env_levels()` and - before parsing CLI options, so an explicit `--log-level` on the command line always - overwrites it afterward — same precedence pattern already used for `SPDLOG_LEVEL`. + falling back to `$HOME/.config/slocker-lite/config.yaml`). Supported `global` keys: + `log-level`, and six `unshare-` keys (`unshare-user`/`unshare-ipc`/ + `unshare-pid`/`unshare-net`/`unshare-uts`/`unshare-cgroup`, one per + `bwrap.cpp`'s own `namespace_probes` entry) controlling whether `-r/--run` + requests each of bwrap's `--unshare-xxx` flags — every other long option is a + one-shot flag, not a setting, so it doesn't belong in a persistent config file. + Each `unshare-*` key accepts `"1"`/`"on"`/`"yes"`/`"true"` (enabled) or + `"0"`/`"off"`/`"no"`/`"false"` (disabled), case-insensitively (`.cpp`-local + `parse_bool_flag()`); an unset key defaults to enabled, and an unrecognized + value logs a `spdlog::warn` and is treated as unset (default enabled) rather + than failing the whole config load — consistent with this file's existing + forward-compatible/ignore-malformed-entries policy (only malformed *YAML + syntax* is a hard error). A missing file returns a default-constructed + (empty) `AppConfig`, not an error; unknown sections/keys (and malformed + individual volume entries) are likewise ignored for forward-compatibility. + `main()` applies `config->log_level` (via the existing `apply_log_level()`) + right after `spdlog::cfg::load_env_levels()` and before parsing CLI options, + so an explicit `--log-level` on the command line always overwrites it + afterward — same precedence pattern already used for `SPDLOG_LEVEL`. `write_config_file()` writes the whole file back out (via libyaml's document-building/emitter API, symmetric to the read side) — used by `-v/--volume` (`create_volume_command()`, `commands.cpp`) to persist a new - `VolumeEntry {name, directory}` into the `volumes` section, preserving `global` - untouched. **`VolumeEntry`/the `volumes` section is a distinct concept from - `OciImageConfig::volumes`**: this is a user-defined `name -> host directory` - mapping created via `-v/--volume`, not an image's own declared mount points (still - unconsumed, see `oci_image.{h,cpp}` above). `AppConfig`/`config.volumes` is looked - up by name in `resolve_volume_mount()` (`volume_mount.{h,cpp}`, see below), which - is how `-r/--run`'s own `-v` usage finds a named volume's host directory. + `VolumeEntry {name, directory}` into the `volumes` section, preserving + `global` (including any set `unshare-*` keys, re-serialized as canonical + `"true"`/`"false"`) untouched. **`VolumeEntry`/the `volumes` section is a + distinct concept from `OciImageConfig::volumes`**: this is a user-defined + `name -> host directory` mapping created via `-v/--volume`, not an image's + own declared mount points (still unconsumed, see `oci_image.{h,cpp}` above). + `AppConfig`/`config.volumes` is looked up by name in `resolve_volume_mount()` + (`volume_mount.{h,cpp}`, see below), which is how `-r/--run`'s own `-v` usage + finds a named volume's host directory. `run_container()` (`commands.cpp`) + resolves the six `unshare-*` fields (each `value_or(true)`) into a + `NamespaceConfig` (`bwrap.h`, see below) once, up front, and passes it to + `run_bwrap()` — `bwrap.{h,cpp}` itself has no dependency on this file or on + YAML parsing at all, only on the already-resolved, defaults-applied struct. - `volume_mount.{h,cpp}` — `is_valid_volume_name()` (no `/`, checked by both `create_volume_command()` and to tell a `-v` spec's name/path apart) and `resolve_volume_mount()`, called once per `-v` occurrence from `run_container()` diff --git a/README.md b/README.md index d742f37..b34abca 100644 --- a/README.md +++ b/README.md @@ -167,17 +167,32 @@ organized into sections: ```yaml global: log-level: debug + unshare-user: on + unshare-ipc: on + unshare-pid: on + unshare-net: on + unshare-uts: on + unshare-cgroup: on volumes: mydata: /home/user/slocker-volumes/mydata ``` -`global.log-level` is the only standing preference supported today (one-shot -commands like `--mount`/`--run`/`--user` don't belong in a config file). An explicit -`--log-level` on the command line always overrides the config file. The `volumes` -section is managed by `-v/--volume` (see above) rather than hand-edited — it's what -`-r/--run`'s own `-v` usage looks named volumes up in. A missing config file is fine -either way (nothing is overridden, and one gets created the first time -`-v/--volume` is used). +`global.log-level` sets the default log verbosity (an explicit `--log-level` on +the command line always overrides it). The six `global.unshare-` keys +control whether `-r/--run` requests the matching bwrap `--unshare-xxx` flag +(only namespace types the running kernel actually supports are ever affected +either way) — each accepts `1`/`on`/`yes`/`true` or `0`/`off`/`no`/`false`, +case-insensitively, and defaults to `on` (enabled) when unset, so the block +above is also the default with nothing configured. **Note on `unshare-net`**: +with no `slirp4netns`-style setup yet implemented, leaving it enabled (the +default) means a sandboxed container currently has no network access at all — +set `unshare-net: off` if you need the sandbox to see the host's network in +the meantime. No other long options belong in a config file (one-shot +commands like `--mount`/`--run`/`--user` don't). The `volumes` section is +managed by `-v/--volume` (see above) rather than hand-edited — it's what +`-r/--run`'s own `-v` usage looks named volumes up in. A missing config file +is fine either way (nothing is overridden, and one gets created the first +time `-v/--volume` is used). ## How it works diff --git a/src/bwrap.cpp b/src/bwrap.cpp index 2f0b22d..80203df 100644 --- a/src/bwrap.cpp +++ b/src/bwrap.cpp @@ -98,6 +98,31 @@ constexpr std::array namespace_probes = {{ {CLONE_NEWCGROUP, "--unshare-cgroup", "cgroup"}, }}; +// Looks up `name`'s policy field in `config` -- a plain if-chain over the six +// known namespace_probes names (mirroring that array), since NamespaceConfig +// (bwrap.h) has one named bool field per type rather than a lookup table. +bool namespace_policy_enabled(const NamespaceConfig& config, std::string_view name) { + if (name == "user") { + return config.user; + } + if (name == "ipc") { + return config.ipc; + } + if (name == "pid") { + return config.pid; + } + if (name == "net") { + return config.net; + } + if (name == "uts") { + return config.uts; + } + if (name == "cgroup") { + return config.cgroup; + } + return true; // unreachable given namespace_probes' fixed set +} + // unshare(2) affects the calling process's own namespaces, so support for each // namespace type is probed in a throwaway forked child rather than the caller. bool kernel_supports_namespace(int clone_flag) { @@ -200,7 +225,8 @@ std::vector build_bwrap_args(const std::string& root, const std::vector& command, const std::vector& volumes, std::optional user, - const std::optional& hostname) { + const std::optional& hostname, + const NamespaceConfig& namespace_config) { // --new-session detaches from the controlling terminal, which breaks job // control for an interactive foreground shell ("can't access tty"). Re-enable // once background/daemonized runs are implemented, where that's the point. @@ -216,12 +242,18 @@ std::vector build_bwrap_args(const std::string& root, auto unshare_args = detect_bwrap_unshare_args(); bool has_uts_ns = false; for (const auto& arg : unshare_args) { - // Not requested yet: without any network setup (slirp4netns or similar), - // unsharing it just leaves the sandbox with no network at all. Re-add once - // network isolation is implemented. (detect_bwrap_unshare_args() still - // probes/reports it, e.g. for -t/--test, since that's kernel capability, not - // policy.) - if (arg == "--unshare-net") { + // Kernel support (detect_bwrap_unshare_args(), above) and policy + // (namespace_config, from global.unshare-* -- config_file.h) are separate + // gates; both must allow a type for it to actually be requested. Note this + // now applies uniformly to net too: previously --unshare-net was always + // dropped here regardless of policy, since without a network setup (e.g. + // slirp4netns) unsharing it just leaves the sandbox with no network at all -- + // that's now the caller's choice via global.unshare-net (default: enabled, + // meaning no network access until slirp4netns support is added). + auto probe = std::find_if(namespace_probes.begin(), namespace_probes.end(), + [&](const auto& p) { return arg == p.bwrap_arg; }); + if (probe != namespace_probes.end() && !namespace_policy_enabled(namespace_config, probe->name)) { + spdlog::debug("namespace {} disabled via config; not requesting {}", probe->name, arg); continue; } args.push_back(arg); @@ -324,6 +356,7 @@ int run_bwrap(const std::string& root, const std::vector& command, const std::vector& volumes, std::optional user, const std::optional& hostname, const std::string& container_name, const std::vector>& extra_env, + const NamespaceConfig& namespace_config, const std::function& on_bwrap_pid_known) { if (user && !find_priv_drop_helper()) { spdlog::error("could not find the {} helper next to this binary; --user/--group requires it", @@ -331,7 +364,7 @@ int run_bwrap(const std::string& root, const std::vector& command, return -1; } - auto bwrap_args = build_bwrap_args(root, command, volumes, user, hostname); + auto bwrap_args = build_bwrap_args(root, command, volumes, user, hostname, namespace_config); auto argv = wrap_for_root_namespace(root, use_nsenter, bwrap_args); if (!argv) { return -1; diff --git a/src/bwrap.h b/src/bwrap.h index a0278fb..764e065 100644 --- a/src/bwrap.h +++ b/src/bwrap.h @@ -74,21 +74,40 @@ struct ResolvedUser { std::string home; // from the image's /etc/passwd entry for uid, or a sane fallback }; +// Per-namespace-type policy: whether build_bwrap_args() should request bwrap's +// --unshare-xxx flag for each type when the kernel also supports it (see +// detect_bwrap_unshare_args() -- kernel support and this policy are separate +// gates; both must allow a given type for it to actually be requested). Field +// names mirror bwrap.cpp's own namespace_probes entries. Resolved from +// AppConfig's global.unshare-* keys (config_file.h) by the caller (run_container(), +// commands.cpp -- defaulting an unset key to true), not read from config directly +// here, so this file has no dependency on config_file.h/YAML parsing. +struct NamespaceConfig { + bool user = true; + bool ipc = true; + bool pid = true; + bool net = true; + bool uts = true; + bool cgroup = true; +}; + // Assembles the full bwrap argv (program name included) to run `command` with // `root` bound as the sandbox's filesystem root, using whichever --unshare-xxx -// flags the kernel supports (see detect_bwrap_unshare_args()). Each entry in -// `volumes` is bound writably at its container_path (see resolve_volume_mount() -// in volume_mount.h). If `user` is set, the command is wrapped so it drops to that -// uid/gid before running -- see run_bwrap() for how, since bwrap's own --uid/--gid -// require --unshare-user, which isn't requested when running as root (see -// detect_bwrap_unshare_args()). If `hostname` is set and the kernel supports -// --unshare-uts (bwrap refuses --hostname without it), passes it as bwrap's own -// --hostname; otherwise logs a warning and leaves the sandbox's hostname alone. +// flags the kernel supports (see detect_bwrap_unshare_args()) and `namespace_config` +// allows. Each entry in `volumes` is bound writably at its container_path (see +// resolve_volume_mount() in volume_mount.h). If `user` is set, the command is +// wrapped so it drops to that uid/gid before running -- see run_bwrap() for how, +// since bwrap's own --uid/--gid require --unshare-user, which isn't requested when +// running as root (see detect_bwrap_unshare_args()). If `hostname` is set and the +// kernel supports --unshare-uts (bwrap refuses --hostname without it), passes it as +// bwrap's own --hostname; otherwise logs a warning and leaves the sandbox's +// hostname alone. std::vector build_bwrap_args(const std::string& root, const std::vector& command, const std::vector& volumes, std::optional user, - const std::optional& hostname); + const std::optional& hostname, + const NamespaceConfig& namespace_config); // Runs bwrap against `root` (the merged mount path from mount_layer()) in the // foreground and waits for it to exit. If `use_nsenter` is true, first locates the @@ -100,8 +119,8 @@ std::vector build_bwrap_args(const std::string& root, // (observed on kernels older than 4.18, per fuse-overlayfs's own release notes). // If `user` is set, this process's own binary is bind-mounted into the sandbox and // used to drop privileges to that uid/gid before running `command` -- see -// build_bwrap_args(). `hostname`, if set, is forwarded to build_bwrap_args() -- -// see there for when it does/doesn't take effect. While bwrap is running, +// build_bwrap_args(). `hostname` and `namespace_config`, if set, are forwarded to +// build_bwrap_args() -- see there for when/how they take effect. While bwrap is running, // `container_name` (paired with its actual pid) is recorded as a locked session // pid file under $XDG_STATE_HOME (see pid_file.h) -- removed again once it exits. // The sandboxed command's environment is built directly here (build_sandbox_env()), @@ -117,4 +136,5 @@ int run_bwrap(const std::string& root, const std::vector& command, const std::vector& volumes, std::optional user, const std::optional& hostname, const std::string& container_name, const std::vector>& extra_env, + const NamespaceConfig& namespace_config, const std::function& on_bwrap_pid_known = nullptr); diff --git a/src/commands.cpp b/src/commands.cpp index 8fc80c6..9b86a79 100644 --- a/src/commands.cpp +++ b/src/commands.cpp @@ -463,6 +463,12 @@ int run_container(const std::filesystem::path& image_tar, : std::vector{"/bin/sh"}; } + NamespaceConfig namespace_config{ + app_config.unshare_user.value_or(true), app_config.unshare_ipc.value_or(true), + app_config.unshare_pid.value_or(true), app_config.unshare_net.value_or(true), + app_config.unshare_uts.value_or(true), app_config.unshare_cgroup.value_or(true), + }; + std::function on_bwrap_pid_known; if (daemonize_flag) { on_bwrap_pid_known = [&](pid_t pid) { report_daemon_started(container_name, pid); }; @@ -471,7 +477,7 @@ int run_container(const std::filesystem::path& image_tar, int exit_code = -1; if (ok) { exit_code = run_bwrap(mounted->merged_path, command, use_nsenter, volume_mounts, resolved_user, hostname, - container_name, *resolved_env, on_bwrap_pid_known); + container_name, *resolved_env, namespace_config, on_bwrap_pid_known); if (exit_code < 0) { spdlog::error("failed to run bwrap"); } diff --git a/src/config_file.cpp b/src/config_file.cpp index 44a132f..aca5eb9 100644 --- a/src/config_file.cpp +++ b/src/config_file.cpp @@ -16,8 +16,13 @@ #include "config_file.h" +#include +#include +#include #include +#include #include +#include #include #include @@ -28,6 +33,40 @@ std::string_view scalar_value(const yaml_node_t& node) { return {reinterpret_cast(node.data.scalar.value), node.data.scalar.length}; } +// Accepts the usual truthy/falsy string forms, case-insensitively. Returns +// nullopt for anything else -- the caller logs and leaves the field unset +// (equivalent to "enabled", the default for every global.unshare-* key) +// rather than treating a typo as a hard config-file error. +std::optional parse_bool_flag(std::string_view value) { + std::string lower(value); + std::transform(lower.begin(), lower.end(), lower.begin(), + [](unsigned char c) { return static_cast(std::tolower(c)); }); + if (lower == "1" || lower == "on" || lower == "yes" || lower == "true") { + return true; + } + if (lower == "0" || lower == "off" || lower == "no" || lower == "false") { + return false; + } + return std::nullopt; +} + +// Pairs a global.unshare-* YAML key with the AppConfig field it fills, so +// load_config_file()/write_config_file() can share one list instead of +// repeating all six keys twice. +struct UnshareKey { + const char* key; + std::optional AppConfig::*field; +}; + +constexpr std::array unshare_keys = {{ + {"unshare-user", &AppConfig::unshare_user}, + {"unshare-ipc", &AppConfig::unshare_ipc}, + {"unshare-pid", &AppConfig::unshare_pid}, + {"unshare-net", &AppConfig::unshare_net}, + {"unshare-uts", &AppConfig::unshare_uts}, + {"unshare-cgroup", &AppConfig::unshare_cgroup}, +}}; + // Finds `key` in a YAML_MAPPING_NODE and returns its value node, or nullptr if // `node` isn't a mapping or has no such (scalar) key. const yaml_node_t* find_in_mapping(yaml_document_t& document, const yaml_node_t& node, @@ -98,6 +137,19 @@ std::optional load_config_file(const std::filesystem::path& path) { config.log_level = std::string(scalar_value(*log_level)); } } + for (const auto& unshare_key : unshare_keys) { + const yaml_node_t* value = find_in_mapping(document, *global, unshare_key.key); + if (!value || value->type != YAML_SCALAR_NODE) { + continue; + } + std::string_view raw = scalar_value(*value); + if (auto parsed = parse_bool_flag(raw)) { + config.*unshare_key.field = *parsed; + } else { + spdlog::warn("config file: unrecognized value '{}' for global.{}, ignoring (leaving enabled)", + raw, unshare_key.key); + } + } } if (const yaml_node_t* volumes = find_in_mapping(document, *root, "volumes")) { if (volumes->type == YAML_MAPPING_NODE) { @@ -132,10 +184,21 @@ bool write_config_file(const std::filesystem::path& path, const AppConfig& confi int root = add_mapping(document); - if (config.log_level) { + bool has_unshare_override = + std::any_of(unshare_keys.begin(), unshare_keys.end(), + [&](const auto& unshare_key) { return (config.*unshare_key.field).has_value(); }); + if (config.log_level || has_unshare_override) { int global = add_mapping(document); - yaml_document_append_mapping_pair(&document, global, add_scalar(document, "log-level"), - add_scalar(document, *config.log_level)); + if (config.log_level) { + yaml_document_append_mapping_pair(&document, global, add_scalar(document, "log-level"), + add_scalar(document, *config.log_level)); + } + for (const auto& unshare_key : unshare_keys) { + if (auto value = config.*unshare_key.field) { + yaml_document_append_mapping_pair(&document, global, add_scalar(document, unshare_key.key), + add_scalar(document, *value ? "true" : "false")); + } + } yaml_document_append_mapping_pair(&document, root, add_scalar(document, "global"), global); } diff --git a/src/config_file.h b/src/config_file.h index 46444f2..055ed0f 100644 --- a/src/config_file.h +++ b/src/config_file.h @@ -31,12 +31,26 @@ struct VolumeEntry { }; // Fields that make sense to persist across invocations (one-shot flags like -// -m/-r/--user don't belong here). Only the "global" section's log-level and the -// "volumes" section are supported today; add more optional fields as more long -// options gain config-file support. +// -m/-r/--user don't belong here). Only the "global" section's log-level/ +// unshare-* keys and the "volumes" section are supported today; add more +// optional fields as more long options gain config-file support. struct AppConfig { std::optional log_level; // global.log-level - std::vector volumes; // volumes section: name -> directory + + // Policy for bwrap's --unshare-xxx flags (src/bwrap.h's NamespaceConfig, + // resolved from these by run_container() -- src/commands.cpp): whether to + // request each namespace type when the kernel also supports it. unset + // means enabled (the default for all six). Accepts "1"/"on"/"yes"/"true" + // and "0"/"off"/"no"/"false", case-insensitively -- see parse_bool_flag() + // in config_file.cpp. + std::optional unshare_user; // global.unshare-user (default: enabled) + std::optional unshare_ipc; // global.unshare-ipc (default: enabled) + std::optional unshare_pid; // global.unshare-pid (default: enabled) + std::optional unshare_net; // global.unshare-net (default: enabled) + std::optional unshare_uts; // global.unshare-uts (default: enabled) + std::optional unshare_cgroup; // global.unshare-cgroup (default: enabled) + + std::vector volumes; // volumes section: name -> directory }; // $XDG_CONFIG_HOME/slocker-lite/config.yaml, or $HOME/.config/slocker-lite/config.yaml