Add launch_custom subprocess dispatcher for CustomResourceConfig#626
Add launch_custom subprocess dispatcher for CustomResourceConfig#626kmontemayor2-sc wants to merge 4 commits intomainfrom
Conversation
Introduces a new `oneof` arm on `TrainerResourceConfig` / `InferencerResourceConfig` that lets callers describe a launcher as a shell command + positional args, instead of a fixed-shape Vertex AI / KFP / local resource config. The proto carries no semantics here — the dispatcher is added in a follow-up PR; this commit only ships the message, regenerated bindings, and the wrapper-property update so downstream code can read `wrapper.trainer_config` and get a `CustomResourceConfig` back. The diff includes a long tail of cosmetic Scala changes outside `gigl_resource_config/` because scalapbc regenerates every sibling proto's emitted source whenever any one proto in the same directory changes. Reviewers can scope to `CustomResourceConfig.scala` and the `*ResourceConfig.scala` siblings that gain the new oneof case. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
/all_test |
GiGL Automation@ 23:08:16UTC : 🔄 @ 23:12:40UTC : ❌ Workflow failed. |
GiGL Automation@ 23:08:18UTC : 🔄 @ 23:14:32UTC : ❌ Workflow failed. |
GiGL Automation@ 23:08:18UTC : 🔄 @ 23:18:08UTC : ✅ Workflow completed successfully. |
GiGL Automation@ 23:08:22UTC : 🔄 @ 23:10:11UTC : ✅ Workflow completed successfully. |
GiGL Automation@ 23:08:24UTC : 🔄 @ 24:38:24UTC : ✅ Workflow completed successfully. |
GiGL Automation@ 23:08:25UTC : 🔄 @ 24:24:39UTC : ✅ Workflow completed successfully. |
|
|
||
| shell_line = " ".join([command, *(shlex.quote(a) for a in args)]) | ||
| logger.info(f"Launching {component.name} via subprocess: {shell_line!r}") | ||
| subprocess.run(shell_line, shell=True, check=True) |
There was a problem hiding this comment.
Semgrep identified an issue in your code:
The subprocess.run() call uses shell=True on user-controlled command strings, allowing shell injection attacks that could execute arbitrary commands with the process's privileges.
More details about this
The subprocess.run() call executes shell_line with shell=True, which spawns a shell process to interpret the command. This is dangerous because if resolved_command or any element in resolved_args comes from untrusted input (e.g., user-provided configuration, external data), an attacker can inject arbitrary shell commands.
For example, if an attacker controls custom_resource_config.command and sets it to id; rm -rf /, the shell will execute both id and the destructive rm -rf / command. Even though shlex.quote() escapes individual arguments, it doesn't protect against injection in the command itself—only in the args list. An attacker who controls the command field can bypass this protection entirely.
Exploit scenario:
- Attacker provides
custom_resource_config.command = "echo test; cat /etc/passwd #" - After joining with args,
shell_linebecomes something like"echo test; cat /etc/passwd #" - When
subprocess.run()executes this withshell=True, the shell interprets the semicolon as a command separator - Both
echo testandcat /etc/passwdexecute, leaking sensitive system files - If the process runs with elevated privileges, the attacker can exfiltrate or modify sensitive data
The shell inherits environment variables and settings from the parent process, which further expands the attack surface. Using shell=False would treat the entire string as a single command name rather than allowing shell metacharacters to be interpreted.
To resolve this comment:
💡 Follow autofix suggestion
| subprocess.run(shell_line, shell=True, check=True) | |
| subprocess.run(shell_line, shell=False, check=True) |
View step-by-step instructions
- Replace
subprocess.run(shell_line, shell=True, check=True)with an invocation that does not useshell=True. - Change the code to directly pass the command and arguments as a list, rather than joining into a string. Use:
subprocess.run([resolved_command] + resolved_args, check=True). - Remove any uses of
shlex.quotewhen building the argument list, since quoting is only necessary when passing a string to the shell. - Ensure that
resolved_commandand every item inresolved_argsare unquoted strings representing the command and its arguments.
Passing the command and arguments as a list with shell=False (the default) is safer, because it avoids any interpretation by a shell, preventing command injection vulnerabilities.
💬 Ignore this finding
Reply with Semgrep commands to ignore this finding.
/fp <comment>for false positive/ar <comment>for acceptable risk/other <comment>for all other reasons
Alternatively, triage in Semgrep AppSec Platform to ignore the finding created by subprocess-shell-true.
You can view more details about this finding in the Semgrep AppSec Platform.
357c3cc to
2ef62bd
Compare
CustomResourceConfig is launcher-pluggable — there is no concrete machine spec to validate against (no machine_type, num_workers, GPU config, etc.). The wrapper's trainer_config / inferencer_config properties now return a union that includes CustomResourceConfig (introduced earlier in this PR), but _validate_machine_config does not accept it. Add an isinstance early-return guard at each call site that logs the skip and returns. Shape and backend-compatibility validation for CustomResourceConfig come in a follow-up PR; this commit only makes the existing validation flow type-clean against the widened union. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2ef62bd to
837dbc5
Compare
Implements `launch_custom`, a thin shim that takes a populated
`CustomResourceConfig` and shells out via
`subprocess.run(shell=True, check=True)`. The proto's `command` is
a shell snippet (so leading `KEY=VALUE` env assignments parse
naturally) and `args[]` are individually `shlex.quote`-d before
joining, so values containing whitespace survive the shell pass.
The dispatcher performs no template substitution: `command` and
`args[]` are taken verbatim, and any placeholder text reaches
`subprocess.run` literally. Consumers that want runtime-context
substitution (e.g. ${gigl:foo}) should resolve it at YAML-load
time before the proto reaches this module. No call site in the
rest of the repo invokes `launch_custom` yet — wiring is added in
a follow-up PR.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
837dbc5 to
066c473
Compare
Implements
launch_custom, a thin shim that takes a populatedCustomResourceConfigand shells out viasubprocess.run(shell=True, check=True). The proto'scommandisa shell snippet (so leading
KEY=VALUEenv assignments parsenaturally) and
args[]are individuallyshlex.quote-d beforejoining, so values containing whitespace survive the shell pass.
The dispatcher performs no template substitution:
commandandargs[]are taken verbatim, and any placeholder text reachessubprocess.runliterally. Consumers that want runtime-contextsubstitution (e.g. ${gigl:foo}) should resolve it at YAML-load
time before the proto reaches this module. No call site in the
rest of the repo invokes
launch_customyet — wiring is added ina follow-up PR.