feat(unstable): allow specifing gid and uid for subprocess #11586
Conversation
cli/dts/lib.deno.ns.d.ts
Outdated
| * On unix `opt.gid` and `opt.uid` can be used to define the gid and uid for | ||
| * the subprocess. |
There was a problem hiding this comment.
I don't think it's going to be clear to most users what this means. Can I suggest copying the description from libstd's CommandExt?
Sets the child process’s user ID. This translates to a setuid call in the child process. Failure in the setuid call will cause the spawn to fail.
And:
Similar to uid, but sets the group ID of the child process. This has the same semantics as the uid field.
I'd argue changing uid/gid without being able to drop group membership is insecure (a mistake I made in libuv) but .groups() is experimental in libstd and tokio doesn't support it at all.
There was a problem hiding this comment.
@crowlKats please update doc with Ben's suggestion.
I'd argue changing uid/gid without being able to drop group membership is insecure (a mistake I made in libuv) but .groups() is experimental in libstd and tokio doesn't support it at all.
@bnoordhuis what would you suggest in this case?
There was a problem hiding this comment.
Either hold off on merging until Tokio grows the right APIs or add a really big warning to the documentation.
The problem with a warning is that it's not actionable. There's no way to detect an insecure situation and act on it.
There was a problem hiding this comment.
Thanks, as you point out the second option is not actionable. Let's hold off merging, and open an feature request to tokio.
There was a problem hiding this comment.
I looked at how libstd implements Command::spawn() (which tokio wraps) and it calls setgroups(0, null()) to drop group membership (barring bug rust-lang/rust#88716) so, on second thought, this is probably good to merge.
If you wanted to be extra secure, you could add a Unix-ony pre_exec hook that calls setgroups(0, null()).
There was a problem hiding this comment.
@crowlKats could you add that call to pre_exec that Ben pointed out?
Closes #5506