Skip to content

fix(cli): validate cluster machine id and labels before they reach the remote shell - #2861

Draft
phil-opp wants to merge 1 commit into
mainfrom
claude/dreamy-bardeen-so9lfs-cluster-id-validation
Draft

fix(cli): validate cluster machine id and labels before they reach the remote shell#2861
phil-opp wants to merge 1 commit into
mainfrom
claude/dreamy-bardeen-so9lfs-cluster-id-validation

Conversation

@phil-opp

Copy link
Copy Markdown
Collaborator

Issue

dora cluster up interpolates each machine.id and every label key/value verbatim into the command it runs over SSH (binaries/cli/src/command/cluster/up.rs, via format_labels_arg in mod.rs):

nohup dora daemon --machine-id {id} --labels {k}={v},... > /tmp/dora-daemon-{id}.log 2>&1 &

ClusterConfig::validate() only checked that the id was non-empty and unique, so a value containing whitespace or shell metacharacters would corrupt that command and the log path:

  • id: "a b" word-splits the daemon arguments.
  • a label value like gpu: "x;rm -rf /" — or a $(...)/backtick in the id — executes arbitrary commands on the remote host.

The cluster.yml is operator-authored, so this is a robustness / footgun issue rather than a remote-exploit path, but a malformed value fails confusingly (or dangerously) instead of being rejected up front.

Fix

Restrict machine.id and every label key/value to the same safe identifier charset dora already enforces for node and data ids — [a-zA-Z0-9_.-] (see libraries/message/src/id.rs) — via a shared validate_shell_safe helper called from validate().

host / user are deliberately left unrestricted: they are network addresses that legitimately carry characters outside this set (e.g. IPv6 literals) and are folded into the ssh target rather than the remote command string. Hostname-style ids and normal labels (jetson-01, gpu=true, arch=arm64) remain valid.

Validation

  • Added tests: a rejected id with shell metacharacters, a rejected malicious label value, and accepted conventional ids. All existing cluster::config tests still pass (16 total).
  • cargo test -p dora-cli, cargo clippy -p dora-cli --all-targets -- -D warnings, and cargo fmt --all -- --check all pass.

🤖 This is a machine-generated pull request opened by Claude Code. A human maintainer should review before merging.

🤖 Generated with Claude Code

https://claude.ai/code/session_0142J7B2578ixrh77kNDNJry


Generated by Claude Code

…e remote shell

`dora cluster up` interpolates each `machine.id` and every label
key/value verbatim into the command it runs over SSH:

    nohup dora daemon --machine-id {id} --labels {k}={v},... \
        > /tmp/dora-daemon-{id}.log 2>&1 &

`ClusterConfig::validate()` only checked that the id was non-empty and
unique, so a value containing whitespace or shell metacharacters would
corrupt that command and the log path — e.g. `id: "a b"` word-splits the
arguments, and a label value like `x;rm -rf /` (or a `$(...)`/backtick
in the id) executes arbitrary commands on the remote host.

Restrict `machine.id` and every label key/value to the same safe
identifier charset dora already enforces for node and data ids
(`[a-zA-Z0-9_.-]`, see `libraries/message/src/id.rs`) via a shared
`validate_shell_safe` helper. `host`/`user` are deliberately left
unrestricted: they are network addresses that legitimately carry other
characters (e.g. IPv6 literals) and are folded into the ssh *target*,
not the remote command string. Hostname-style ids and normal labels
(`jetson-01`, `gpu=true`, `arch=arm64`) remain valid. Adds tests for a
rejected id, a rejected label value, and the accepted cases.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0142J7B2578ixrh77kNDNJry
@trunk-io

trunk-io Bot commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Merging to main in this repository is managed by Trunk.

  • To merge this pull request, check the box to the left or comment /trunk merge below.

After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here

Copy link
Copy Markdown
Collaborator Author

Automated review by Claude — this is a fully automated review; no human has vetted it.

No issues found. Restricting machine.id and label keys/values to [a-zA-Z0-9_.-] closes the unquoted-interpolation footgun in the SSH command cluster up builds, and matches the charset dora already enforces for node/data ids. Leaving host/user unrestricted is reasonable since they form the ssh target rather than the remote command string and legitimately carry IPv6 literals. The added tests exercise the real ClusterConfig::load path for both rejection and acceptance cases.


Generated by Claude Code

Copy link
Copy Markdown
Collaborator Author

🤖 Automated review by Claude — this review is fully automated and has not been vetted by a human.

The machine.id / label validation itself looks correct, and the added tests drive the real ClusterConfig::load path for both the rejected and accepted cases. But I think the fix is incomplete:

  • zenoh_peer is left unvalidated even though it is interpolated into the same remote shell command. zenoh_peer (a free-form Option<String> in config.rs:23) is folded into the daemon command via format_zenoh_peer_argformat!(" --zenoh-peer {ep}") (mod.rs:86), and that string is embedded unquoted into exactly the command this PR is hardening — up.rs:70 (... {zenoh_peer_arg}{labels} ... > /tmp/dora-daemon-{id}.log) and the systemd ExecStart line at install.rs:50. So a config such as zenoh_peer: "tcp/1.2.3.4:5456 ; rm -rf /" (or a $(...)/backtick) reproduces the very command-injection / word-splitting this PR closes for id and labels. The PR description calls out only host/user as the intentionally-unrestricted fields, so zenoh_peer appears to have been overlooked rather than deliberately excluded. It runs the same validate_shell_safe gauntlet — though note the current charset [a-zA-Z0-9_.-] rejects / and :, which a zenoh endpoint like tcp/10.0.0.1:5456 legitimately needs, so it would need its own slightly wider (but still whitespace-/metacharacter-free) charset rather than the identifier one.

Minor, pre-existing and out of scope for this PR: host is passed to ssh as a separate argv rather than through a shell, so shell metacharacters there are inert — but a host value beginning with - could still be parsed by ssh as an option. Noting only for completeness.


Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants