Add user: support to compose services
ComposeService::user/group (compose_file.{h,cpp}) parse a "user[:group]"
key, split on the first ':' the same way an image's own declared USER is
split (oci_image.cpp). start_compose_services() passes both straight
through to run_mounted_container()'s existing user/group parameters, which
already fall back to the image's own declared user when unset -- the same
default -r/--run itself has when --user isn't given.
Verified end to end on the real target machine (root, via the scoped doas
rule), checked via `ps -eo pid,ppid,uid,cmd` (not -x/--exec, see below):
the actual sandboxed command runs as the resolved uid/gid, matching plain
-r --user's own already-working behavior.
Also recorded in TODO.md: verifying this surfaced a real but unrelated
bug in resolve_namespace_pid() (sandbox_process.cpp), which -x/--exec's
own default-identity resolution uses -- it stops at bwrap's own pid-1
namespace supervisor instead of walking one level deeper to the real
(correctly priv-dropped) target, so `-x/--exec <pid> -- id` with no
explicit --user misreports root for a session that's actually running as
a non-root user the whole time. Not a regression from this change and not
fixed here.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Gv3s5jckJKzh6JkMoi2Akz
This commit is contained in:
@@ -2678,9 +2678,14 @@ Source layout (all under `src/`):
|
|||||||
since Compose's own `ports:` syntax has none, so a later `-p`
|
since Compose's own `ports:` syntax has none, so a later `-p`
|
||||||
resolution picks the service's own sole `extern` network, the same
|
resolution picks the service's own sole `extern` network, the same
|
||||||
"unqualified `-p`" default an ordinary CLI `-p` already has. An
|
"unqualified `-p`" default an ordinary CLI `-p` already has. An
|
||||||
explicit `--user`/`--group` still isn't wired up for compose services
|
explicit `--user`/`--group`, wired the same way: `user: "user[:group]"`
|
||||||
— every service runs as whatever user its own image declares, same as
|
(`ComposeService::user`/`group`, `compose_file.h` — split on the
|
||||||
`-r/--run`'s own default.
|
*first* `':'`, the exact same way an image's own declared `USER` is
|
||||||
|
split, `oci_image.cpp`'s own `read_oci_image_config()`) is passed
|
||||||
|
straight through to `run_mounted_container()`'s own `user`/`group`
|
||||||
|
parameters; when unset, that function already falls back to the
|
||||||
|
image's own declared user, the exact same default `-r/--run` has when
|
||||||
|
`--user` isn't given.
|
||||||
5. `compose_state_file_path()`/`record_compose_services()` — write the
|
5. `compose_state_file_path()`/`record_compose_services()` — write the
|
||||||
compose file's own real, absolute path as a header line (needed
|
compose file's own real, absolute path as a header line (needed
|
||||||
because `compose_state_file_path()`'s own *filename* only encodes a
|
because `compose_state_file_path()`'s own *filename* only encodes a
|
||||||
|
|||||||
@@ -130,3 +130,32 @@ project's own state should stay root-only or move too. Not urgent -- this
|
|||||||
project's networking is already root-only in every other respect (bridges,
|
project's networking is already root-only in every other respect (bridges,
|
||||||
iptables, etc.), so this is a narrower, lower-priority hardening pass for
|
iptables, etc.), so this is a narrower, lower-priority hardening pass for
|
||||||
later, not a blocker for anything currently in progress.
|
later, not a blocker for anything currently in progress.
|
||||||
|
|
||||||
|
## `-x/--exec`/`--kill`'s `resolve_namespace_pid()` can stop one level too shallow when `--user` used priv-drop
|
||||||
|
|
||||||
|
Found while verifying compose's new `user:` support (root, via the scoped
|
||||||
|
`doas` rule): a session started with `--user` (root, `--unshare-pid` in
|
||||||
|
effect, the default) has a real process tree of `outer bwrap` -> `bwrap's
|
||||||
|
own pid-1 supervisor for the new pid namespace` (never itself execs into
|
||||||
|
anything -- it's what keeps the namespace alive, since the kernel requires
|
||||||
|
some process to be pid 1) -> the actual target, forked by that supervisor,
|
||||||
|
which execs `slocker-lite-priv-drop` and then the real command. Confirmed
|
||||||
|
via `ps -eo pid,ppid,uid,cmd` that the real command genuinely runs as the
|
||||||
|
resolved uid/gid (priv-drop itself is correct, not the bug) -- but
|
||||||
|
`resolve_namespace_pid()` (`sandbox_process.{h,cpp}`), used by both
|
||||||
|
`-x/--exec`'s own default-identity resolution and `--kill`, stops at the
|
||||||
|
pid-1 supervisor (its namespaces already differ from the true outer pid,
|
||||||
|
satisfying that function's own single-hop comparison) instead of walking
|
||||||
|
one level deeper to the real target. Concretely: `-x/--exec <pid> -- id`
|
||||||
|
with no explicit `--user` on a session that resolved a non-root user via
|
||||||
|
priv-drop incorrectly reports `root` (reads the supervisor's own
|
||||||
|
still-root `/proc/.../status`), even though the session's real command is
|
||||||
|
genuinely running as the resolved non-root user the whole time.
|
||||||
|
|
||||||
|
**Not urgent**: `-x/--exec --user <explicit>` and `--kill` are unaffected
|
||||||
|
in practice (`--kill`'s own cgroup/pid-namespace strategies already reach
|
||||||
|
every process regardless of tree depth; only `-x/--exec`'s *default*
|
||||||
|
identity guess is off). Needs a real fix to `resolve_namespace_pid()` (or
|
||||||
|
a documented, narrower workaround) to correctly walk past bwrap's own
|
||||||
|
pid-1 supervisor when `--unshare-pid` is in effect, rather than assuming
|
||||||
|
the first namespace-differing child is the real target.
|
||||||
|
|||||||
@@ -518,6 +518,22 @@ std::optional<ComposeFile> load_compose_file(const std::filesystem::path& path)
|
|||||||
service.container_name = std::string(scalar_value(*container_name_node));
|
service.container_name = std::string(scalar_value(*container_name_node));
|
||||||
}
|
}
|
||||||
|
|
||||||
|
if (const yaml_node_t* user_node = find_in_mapping(document, *value_node, "user")) {
|
||||||
|
if (user_node->type != YAML_SCALAR_NODE || scalar_value(*user_node).empty()) {
|
||||||
|
spdlog::error("compose file {}: service '{}' user must be a non-empty string", path.string(),
|
||||||
|
service_name);
|
||||||
|
return std::nullopt;
|
||||||
|
}
|
||||||
|
std::string user_spec(scalar_value(*user_node));
|
||||||
|
auto colon = user_spec.find(':');
|
||||||
|
if (colon == std::string::npos) {
|
||||||
|
service.user = user_spec;
|
||||||
|
} else {
|
||||||
|
service.user = user_spec.substr(0, colon);
|
||||||
|
service.group = user_spec.substr(colon + 1);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
if (const yaml_node_t* command_node = find_in_mapping(document, *value_node, "command")) {
|
if (const yaml_node_t* command_node = find_in_mapping(document, *value_node, "command")) {
|
||||||
auto command = parse_command(document, *command_node, service_name);
|
auto command = parse_command(document, *command_node, service_name);
|
||||||
if (!command) {
|
if (!command) {
|
||||||
|
|||||||
@@ -52,6 +52,17 @@ struct ComposeService {
|
|||||||
std::string name; // the mapping key under `services`
|
std::string name; // the mapping key under `services`
|
||||||
std::string image;
|
std::string image;
|
||||||
std::optional<std::string> container_name;
|
std::optional<std::string> container_name;
|
||||||
|
// Parsed from `user: "user[:group]"`, split on the *first* ':' exactly
|
||||||
|
// the same way an image's own declared `USER` is split
|
||||||
|
// (oci_image.cpp's own read_oci_image_config()). Both unset when the
|
||||||
|
// compose file gives no `user:` key at all -- matching `-r/--run`'s
|
||||||
|
// own default when `--user` isn't given, the image's own declared user
|
||||||
|
// is used instead (falling back further to root only if the image
|
||||||
|
// declares none either), handled automatically by
|
||||||
|
// run_mounted_container() (commands.h) since it already implements
|
||||||
|
// that exact fallback for `-r/--run` itself.
|
||||||
|
std::optional<std::string> user;
|
||||||
|
std::optional<std::string> group;
|
||||||
// Always normalized to an argv list: Compose's own list form is used
|
// Always normalized to an argv list: Compose's own list form is used
|
||||||
// as-is; its scalar shell-string form is wrapped as {"sh", "-c",
|
// as-is; its scalar shell-string form is wrapped as {"sh", "-c",
|
||||||
// <string>}, matching Compose's "run through the image's shell"
|
// <string>}, matching Compose's "run through the image's shell"
|
||||||
|
|||||||
@@ -289,7 +289,7 @@ std::vector<ComposeStartedService> start_compose_services(const ComposeFile& com
|
|||||||
// any other service, which is why the only way out of this block
|
// any other service, which is why the only way out of this block
|
||||||
// below is an explicit _exit(), never `continue`.
|
// below is an explicit _exit(), never `continue`.
|
||||||
int exit_code = run_mounted_container(image_tar, mounted, container_name, service.command, use_nsenter,
|
int exit_code = run_mounted_container(image_tar, mounted, container_name, service.command, use_nsenter,
|
||||||
std::nullopt, std::nullopt, dns_hostname, volume_specs,
|
service.user, service.group, dns_hostname, volume_specs,
|
||||||
service.environment_specs, true, network_specs, port_forward_specs,
|
service.environment_specs, true, network_specs, port_forward_specs,
|
||||||
false, config);
|
false, config);
|
||||||
_exit(exit_code < 0 ? 1 : exit_code);
|
_exit(exit_code < 0 ? 1 : exit_code);
|
||||||
|
|||||||
@@ -154,9 +154,11 @@ struct ComposeStartedService {
|
|||||||
// never a network: prefix (Compose's own ports: syntax has no such
|
// never a network: prefix (Compose's own ports: syntax has no such
|
||||||
// qualifier), so a later -p resolution picks the service's own sole
|
// qualifier), so a later -p resolution picks the service's own sole
|
||||||
// `extern` network, the same "unqualified -p" default an ordinary CLI
|
// `extern` network, the same "unqualified -p" default an ordinary CLI
|
||||||
// `-p` already has. An explicit --user/--group isn't wired up for compose
|
// `-p` already has. `service.user`/`service.group` (ComposeService, parsed
|
||||||
// services yet, though -- every service still runs as whatever user its
|
// from a `user: "user[:group]"` key) are passed straight through to
|
||||||
// own image declares, same as -r/--run's own default.
|
// run_mounted_container()'s own `user`/`group` parameters -- when unset,
|
||||||
|
// that function already falls back to the image's own declared user, the
|
||||||
|
// exact same default `-r/--run` itself has when `--user` isn't given.
|
||||||
//
|
//
|
||||||
// Returns one ComposeStartedService per service that actually started --
|
// Returns one ComposeStartedService per service that actually started --
|
||||||
// fewer than compose.services on any partial failure (already logged
|
// fewer than compose.services on any partial failure (already logged
|
||||||
|
|||||||
@@ -125,6 +125,50 @@ TEST_CASE("compose file: container_name -- optional, and duplicates across servi
|
|||||||
CHECK_FALSE(load_compose_file(colliding_with_name).has_value());
|
CHECK_FALSE(load_compose_file(colliding_with_name).has_value());
|
||||||
}
|
}
|
||||||
|
|
||||||
|
TEST_CASE("compose file: user -- optional, with and without a group, split the same way as an image's own USER",
|
||||||
|
"[unit]") {
|
||||||
|
ScratchXdgDirs scratch;
|
||||||
|
|
||||||
|
auto unset = write_compose(scratch.path(),
|
||||||
|
"services:\n"
|
||||||
|
" web:\n"
|
||||||
|
" image: busybox:latest\n");
|
||||||
|
auto loaded_unset = load_compose_file(unset);
|
||||||
|
REQUIRE(loaded_unset.has_value());
|
||||||
|
CHECK_FALSE(loaded_unset->services[0].user.has_value());
|
||||||
|
CHECK_FALSE(loaded_unset->services[0].group.has_value());
|
||||||
|
|
||||||
|
auto user_only = write_compose(scratch.path(),
|
||||||
|
"services:\n"
|
||||||
|
" web:\n"
|
||||||
|
" image: busybox:latest\n"
|
||||||
|
" user: \"1000\"\n");
|
||||||
|
auto loaded_user_only = load_compose_file(user_only);
|
||||||
|
REQUIRE(loaded_user_only.has_value());
|
||||||
|
REQUIRE(loaded_user_only->services[0].user.has_value());
|
||||||
|
CHECK(*loaded_user_only->services[0].user == "1000");
|
||||||
|
CHECK_FALSE(loaded_user_only->services[0].group.has_value());
|
||||||
|
|
||||||
|
auto user_and_group = write_compose(scratch.path(),
|
||||||
|
"services:\n"
|
||||||
|
" web:\n"
|
||||||
|
" image: busybox:latest\n"
|
||||||
|
" user: \"1000:1000\"\n");
|
||||||
|
auto loaded_both = load_compose_file(user_and_group);
|
||||||
|
REQUIRE(loaded_both.has_value());
|
||||||
|
REQUIRE(loaded_both->services[0].user.has_value());
|
||||||
|
CHECK(*loaded_both->services[0].user == "1000");
|
||||||
|
REQUIRE(loaded_both->services[0].group.has_value());
|
||||||
|
CHECK(*loaded_both->services[0].group == "1000");
|
||||||
|
|
||||||
|
auto empty_user = write_compose(scratch.path(),
|
||||||
|
"services:\n"
|
||||||
|
" web:\n"
|
||||||
|
" image: busybox:latest\n"
|
||||||
|
" user: \"\"\n");
|
||||||
|
CHECK_FALSE(load_compose_file(empty_user).has_value());
|
||||||
|
}
|
||||||
|
|
||||||
TEST_CASE("compose file: command -- list form used as-is, scalar form wrapped in sh -c", "[unit]") {
|
TEST_CASE("compose file: command -- list form used as-is, scalar form wrapped in sh -c", "[unit]") {
|
||||||
ScratchXdgDirs scratch;
|
ScratchXdgDirs scratch;
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user