Support setting stdin, stdout and stderr

[jeromegn]

(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 of youki and stdio is inherit. Therefore the spawner can set their own stdio file descriptors for the spawned youki process. That's not the case for us.

This adds a few functions to set stdin, stdout and stderr as either Inherit (default), Null, Pipe or a specific Fd.

It is a breaking change for libcontainer since the return values have changed. This is because we can't clone PipeReader, PipeWriter or Closing as they need to close a file descriptor on Drop.

[utam0k]

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?

[utam0k]

@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.

[YJDoc2]

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.

[YJDoc2]

What is the reason behind adding this check?

[jeromegn]

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.

[YJDoc2]

Hey, this makes sense, but I'd request you to separate this change in its own PR, not mix with this one.

[YJDoc2]

Can we add comments explaining what this and the StdioDescriptors above are used for and how they are used?

[jeromegn]

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 😄

[jeromegn]

I have been pulled on a different project for now, but will get back to this soon (and answer your questions.)

[jeromegn]

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?

If there's a way to do this without modifying libcontainer, I would be happy to do it.

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 libcontainer as a library and not as if I was calling youki as a separate process. If I had to use the CLI, then I'd be adding much complexity and some overhead in acting like a OCI CLI spec user.

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.

[YJDoc2]

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?

[jeromegn]

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.

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.

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.

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 libcontainer users, then that's good with me!

I aimed at providing an API similar to std::process::Command and std::process::Stdio. I couldn't keep the Stdio in the Container because it is clonable and doesn't hold much state at all, so I had to return stdin, stdout and stderr separately. That's the most annoying change.

I didn't think it mattered because I doubt many use libcontainer as a library or else they would've had similar problems as me?

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.

[YJDoc2]

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.

Yes, in that case it is not much of use.

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 libcontainer users, then that's good with me!

👍

I didn't think it mattered because I doubt many use libcontainer as a library or else they would've had similar problems as me?

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 😅

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.

Thank you for taking a look at it 🙏 I'll try over the weekend to see if I can implement what I have suggested.

[utam0k]

Instead of libcontainer doing all the work to create and setup the pipes etc, we give that responsibility to the user. I

+1