-
Notifications
You must be signed in to change notification settings - Fork 344
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support setting stdin, stdout and stderr #2749
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Jerome Gravel-Niquet <jeromegn@gmail.com>
Signed-off-by: Jerome Gravel-Niquet <jeromegn@gmail.com>
Signed-off-by: Jerome Gravel-Niquet <jeromegn@gmail.com>
Signed-off-by: Jerome Gravel-Niquet <jeromegn@gmail.com>
As a first impression, it does not seem bad. But I'm just confident that I understand this PR. To help my experience, may I ask you to give me a specific example or an e2e test? |
@containers/youki-maintainers This is a big new feature for us. Please take a look at this PR even if lightly. Especially, the security aspect is important. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, haven't taken a detailed look, a couple of comments, but importantly : can you provide some code example where what you are trying to do cannot be done via current libcontainer, and this will allow it? I agree with @utam0k, this can have big security implications.
"executable '{}' not found in $PATH", | ||
args[0] | ||
)))?; | ||
if !args[0].contains('/') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the reason behind adding this check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I shouldn't have included it in this PR. My reasoning for the change is that libcontainer shouldn't try to resolve the path for a program containing a /
since it's likely an absolute path or relative path in which case it wouldn't be resolvable with the $PATH variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, this makes sense, but I'd request you to separate this change in its own PR, not mix with this one.
} | ||
} | ||
|
||
fn prepare_stdio_descriptors(fds: &[Fd; 3]) -> Result<StdioDescriptors, LibcontainerError> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add comments explaining what this and the StdioDescriptors
above are used for and how they are used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A lot of this PR was extracted from the unshare
crate as it already implements the feature. I don't perfectly understand every part of it and the code would definitely benefit from some scrutiny 😄
I have been pulled on a different project for now, but will get back to this soon (and answer your questions.) |
If there's a way to do this without modifying The features added here allow usage lke: let (pid, stdio_fds) =
ContainerBuilder::new(container_name.clone(), Default::default())
.with_root_path("/containers")?
.with_stdin(libcontainer::stdio::Stdio::piped())
.with_stdout(libcontainer::stdio::Stdio::piped())
.with_stderr(libcontainer::stdio::Stdio::piped())
.as_tenant()
.with_env(env)
.with_container_args(args)
.with_detach(true)
.build()?;
let stdin = stdio_fds
.stdin
.take().unwrap();
// write to stdin ... Effectively, I need to be able to use Specific use case for this feature: Using my own file descriptors for stdin / stdout and stderr. Right now there's no way to set this. The only option is to get a PTY, but that's not always what we want. Most of the time, we need a /dev/null stdin and we want to capture stdout and stderr to pipe them into a logs receiver. |
Hey, so I took a look at this, and some thoughts : I think you might have checked this already, but can the console_socket be useful here? libcontainer provides options to give a fd, where the fd of the master terminal will be sent. However, it might mix up the std* streams instead of separating them, not sure. Corresponding code is here for setting up the tty and sending fd to master and here to call the function if socket_fd is specified when building the container . Another issue might be that the call to send the master fd might block on receiver, but because we will be calling libcontainer and reading the socket from same process, it might block forever. Not sure need to check. Alternatively, if that doesn't work or not useful enough for this case, we can consider providing the std* fds to libcontainer. However, I'm not particularly happy with the amount of code we need to add, given that it is for this specific feature, and has several security implications. What do you think about the following way - Instead of libcontainer doing all the work to create and setup the pipes etc, we give that responsibility to the user. In the set_std* functions, we directly take the raw fds and in the build call, where we are checking for console fd, we also check if any of the fds are provided and do a setup_console call with those fds. We will need to check that providing console_socket and these fds is mutually exclusive, and error in build() if given both. We might need to do some fnctl calls to properly propagate the fds from the main->intermediate->init process for setting no close exec stuff, but libcontainer will bascially only deal with connecting std* of container process to these fds, and not deal with actually creating these fds. That way it is the responsibility of user to make sure these fds are secure and valid. Furthermore they can setup the std* as they want - pipe, null etc. with default being inherit as it is. I'd also suggest putting all the above behind a feature which is disabled by default, so users (including youki) cannot accidentally use these features, until they explicitly enable them. I don't think these functions (set_std*) should be unsafe, bt we should document the implication in a doc comment warning. wdyt? |
I have checked this, yes. The issue if I don't want a PTY most of the time. I could still use it, but then the environment would run with the assumption that it's interactive and programs can behave differently in these scenarios.
That's fine with me. I don't mind if the API is awkward for us, as long as it's not preventing us from achieving our goals. If you think this is better in terms of API for I aimed at providing an API similar to I didn't think it mattered because I doubt many use In any case, I can maybe figure out how to make what you're suggesting work. However, I am currently looking at alternatives because of #2756 which seems harder to fix. |
Yes, in that case it is not much of use.
👍
There are few projects that do, but maybe they haven't had the need for this. My primary intention with the suggestion was to keep the potential surface of attack as minimal as possible, which is why I am a bit iffy at the changes here 😅
Thank you for taking a look at it 🙏 I'll try over the weekend to see if I can implement what I have suggested. |
+1 |
Maybe we just need to add three function like |
(This is a little rough and I'm happy to make changes.)
We're using
libcontainer
from a single process and we need to be able to set stdio specifically for each forked / cloned process running as containers. Usually each container is also its own invocation ofyouki
and stdio is inherit. Therefore the spawner can set their own stdio file descriptors for the spawnedyouki
process. That's not the case for us.This adds a few functions to set
stdin
,stdout
andstderr
as eitherInherit
(default),Null
,Pipe
or a specificFd
.It is a breaking change for
libcontainer
since the return values have changed. This is because we can't clonePipeReader
,PipeWriter
orClosing
as they need to close a file descriptor onDrop
.