Restrict OCI bind-mount sources to configured directories and check the launch command username

[reiabreu]

run-oci-container parses the launch command file before setup_dir_permissions changes the worker directory's ownership, and requires the command file's username to match the user passed to the worker-launcher. run_oci_container now takes the parsed oci_launch_cmd, which main owns and frees. Bind-mount sources must be absolute paths with no "." or ".." component, equal to or under a directory listed in the new worker.launcher.oci.allowed.mount.source.dirs config key; if none are configured, all sources are rejected.

How this was tested

Built the worker-launcher native tree with autoreconf -i && ./configure && make check (autotools, compiled with -Werror); the test suite passes. Adds test_mount_path_helpers and test_mount_source_allowed_dirs.

[rzo1]

Currently low on time, thus a LLM based review.

Two things worth addressing before merge, plus a couple of minor ones.

1. The mount-source allowlist is lexical-only and is bypassable via a symlink (major).
is_valid_mount_source() validates the source purely as a string — absolute path, no literal ./.. components, textual prefix-match against the configured dirs — and the string is then written verbatim into the runc config. Since there's no realpath/lstat, symlink resolution happens later at mount time (in runc, as root), after the check. A symlink component planted under an allowlisted, user-writable tree — and the docs here recommend allowlisting /data/storm, which contains the worker/artifacts/tmp roots that get chowned to the topology user — resolves to a host path outside every allowed dir (e.g. bind-mounting host /etc rw into the container). It's also a TOCTOU: the string is checked at parse time but resolved at mount time. In the normal flow the supervisor generates the mount sources, so this is primarily defense-in-depth against a tampered launch command (the same threat model the new username check assumes) plus a cross-worker symlink-planting attack, but it does defeat the restriction this PR advertises. Suggest canonicalizing the source with realpath (as the wl user, refusing symlinked components) and applying the containment check to the resolved path, and/or performing the mount via an O_NOFOLLOW-opened fd.

2. Degenerate values of the new worker.launcher.oci.allowed.mount.source.dirs key hit UB (minor).
The new get_values()/free_values() call site inherits two edge cases from the config helpers. A value consisting only of delimiters (...dirs=,) makes extract_values_delim return a non-NULL, non-NULL-terminated, uninitialized array (it only writes the terminator when size > 0) — the loop then reads uninitialized pointers into strncmp, and free_values frees garbage. A leading-comma value (...dirs=,/data/storm) makes free_values call free() on an interior pointer of the strdup'd buffer → abort on every OCI launch. The helper bug is pre-existing (also reachable via banned.users), but this PR adds a new admin-facing key whose plausible typos trip it. Since the trigger is the root-owned config, this is minor, but a defensive check (or fixing extract_values_delim to always NUL-terminate) would avoid it.

3. Configuring / as an allowed dir rejects everything under it (minor).
is_mount_source_under only strips trailing slashes while (allowed_len > 1 ...), so with allowed = "/" the boundary check requires source[1] to be \0 or /; /etc/resolv.conf is rejected. The natural "allow everything" escape hatch instead fails every OCI launch on that node, contradicting the documented "one of these directories or a path underneath one of them". Either special-case the root entry or document that / isn't valid.

4. Tests cover only the helpers, not the enforcement points (minor).
test_mount_path_helpers/test_mount_source_allowed_dirs exercise the helpers directly, but nothing parses a full launch-command JSON — so the hookup at is_valid_mount() (oci_launch_cmd.c:431) could be reverted with the suite still green, and the strcmp(olc->username, user_name) check in main.c has no test at all (main.c isn't linked into the test binary). A parse_oci_launch_cmd-level test for a mount-rejecting and a username-mismatch command file would make a regression of the fix visible.

[reiabreu]

Thank you for the feedback, @rzo1. I'm afraid I haven't touched C in many years, so I relied entirely on Claude for these fixes.

• 1 (symlink bypass): is_valid_mount_source now resolves the source and each configured directory with realpath before the containment check and rejects unresolvable sources. Docs drop the /data/storm example and state that only directories the topology user cannot write to should be listed (runc re-resolves at mount time — residual TOCTOU). Added a symlink-escape test.
• 2 (degenerate config values): Split into Return independently owned value arrays from get_values #9013, since the helper bug affects every get_values caller. There extract_values_delim NUL-terminates the zero-token case and get_values returns independently-owned strings so free_values no longer frees an interior pointer; includes a regression test that aborts on the pre-fix code.
• 3 (/ allowed dir): Documented that / isn't a usable entry (a path is accepted only when it equals or sits under a listed directory as a whole component).
• 4 (enforcement-point tests): The parse_oci_launch_cmd-level and username-mismatch tests need main.c linked into the test binary — same deferred test-harness change.

[reiabreu]

Following up on the test-coverage point: the enforcement paths in main.c (the launch-command username check and the container-id reject dispatch) can't be exercised today because main.c isn't linked into the test binary. I'll cover this in a dedicated follow-up PR once this and the related worker-launcher PRs land on master — it will pull that dispatch logic into a test-linked source and add a parse_oci_launch_cmd-level test (mount-rejection + username-mismatch), rather than stacking a refactor on the still-open branches.

[rzo1]

@reiabreu Can you rebase?

[reiabreu]

@rzo1 Done — rebased onto master (now includes #9006 and #9008). Resolved the overlaps in oci_launch_cmd.h and test-worker-launcher.c (kept validate_container_id alongside the mount-source declarations/tests); make check is green under -Werror.

[reiabreu]

The follow-up test coverage is now up in #9014 — it adds the parse-level mount-source rejection test and the username-mismatch test noted in point 4 (the mount test parses a full launch command that is valid except for its bind-mount source, so it exercises the is_valid_mount hookup, not just is_valid_mount_source).