mount /dev/console

[elijah-wright]

fixes #145

this PR changes configureConsole() to bind mount /dev/console if process.terminal is true. I couldn't think of a better way to get the pty path so I used readlink. most OCI images don't have /dev/console because they don't have mknod and devtmpfs won't create /dev/console without a physical console in the namespace, so it also creates /dev/console

@dcantah

[dcantah]

Nice, will look at this today. The alternative approach that runc takes (although I'd need to remember why, the likely answer is security or correctness) is to make the pty in the "container" (during the setup, but definitely after all the namespaces are up), and then send the fd via a unix domain socket, although we could use pidfd as well, to the parent. That'd be a much more involved change however.

[brauner]

Nice, will look at this today. The alternative approach that runc takes (although I'd need to remember why, the likely answer is security) is to make the pty in the "container" (during the setup, but definitely after all the namespaces are up), and then send the fd via a unix domain socket, although we could use pidfd as well, to the parent. That'd be a much more involved change however.

Nice, will look at this today. The alternative approach that runc takes (although I'd need to remember why, the likely answer is security) is to make the pty in the "container" (during the setup, but definitely after all the namespaces are up), and then send the fd via a unix domain socket, although we could use pidfd as well, to the parent. That'd be a much more involved change however.

If the pty device originated from a devpts instance outside the containers mount namespace resolving that pty will fail with ENODEV in various libc functions confusing some tools.

There's other very security relevant aspects but those only come into play once you start allocating pty FDs when spawning a shell or process in the container. Doing that correctly is a bit more involved as allocating ptys from inside the container must be done via rather involved special purpose APIs.

[elijah-wright]

yeah like @brauner said I wasn't looking at that too much but I think there are some libc helpers that will try to resolve the path of the pty, maybe ptsname_r or ttyname_r but I'd have to look at it. the pty device is created by TerminalIO.create() which happens before the call to /sbin/vmexec or really unshare(CLONE_NEWPID | CLONE_NEWNS | CLONE_NEWUTS)

allocating the pty in the container would avoid working with the container's /dev from the runtime process and there's probably some other stuff, maybe accurate attributes. but I think it's also tricky because if you mount devpts with newinstance then you can't open /dev/pts/* from the host. if you have a pidfd I think you could use pidfd_getfd? if not a Unix socket with SCM_RIGHTS would probably work

[dcantah]

If the pty device originated from a devpts instance outside the containers mount namespace resolving that pty will fail with ENODEV in various libc functions confusing some tools.

Makes sense okay

@elijah-wright Here is what I'm mentioning https://github.com/opencontainers/runc/blob/b1722d790214952e8a20d4ddd6a83451b9b665a1/libcontainer/init_linux.go#L370. The tricky part is like you'd mentioned, the setup for IO for our process type assumes we .start() it before launching the process as we currently create it outside, but now specifically just for the TerminalIO type we need to grab the pty master fd from vmexec and then start the relay.

[dcantah]

if you have a pidfd I think you could use pidfd_getfd? if not a Unix socket with SCM_RIGHTS would probably work

Yep!

[elijah-wright]

do you want me to change the PR to match runc then?

[dcantah]

@elijah-wright Yes, I think it's the right approach.

[elijah-wright]

@dcantah can you look at my new changes

[dcantah]

@elijah-wright Will look at the new push tomorrow morning (or tonight possibly), but can you run make fmt and re-push to make the CI nice and happy 😄

[elijah-wright]

oh ok ty. I'm at work but I can do it in a few hours

[elijah-wright]

@dcantah I think I fixed the build problems

[elijah-wright]

@dcantah I'm stuck on the syscall import

[dcantah]

@elijah-wright I'll take a look, let me pull this locally

[dcantah]

@elijah-wright Seems Swift can't generate bindings for variadic c functions is the problem 😔, so I've written us some wrappers here we can use for this. This should be checked in tomorrow morning I imagine #189. You're going to want to rebase here also as it seems a little behind

[dcantah]

Ok you should be set to rebase on main and grab the new wrappers

[elijah-wright]

@dcantah ok after some trial and error I think it's good, I did some work getting it ready for the new wrappers yesterday but rebase kept unsigning the commits

[elijah-wright]

@dcantah I haven't looked at your changes yet, do you want to implement them here? I can do it in a few hours if not

[dcantah]

Yea I can fool around with it and post a patch we can apply here

[dcantah]

Sorry for delay, the long weekend consumed me 😂. I have this almost functioning and will push a patch here you can apply and we can work together on

[elijah-wright]

no that's ok! do you want me to squash these commits together and then you can apply the patch yourself? if not that's ok. I want you to get credit for what you did

[dcantah]

Squashing now would help, but we can just put a "co-authored by" line on the final commit so it doesn't matter much

[dcantah]

@elijah-wright I ended up posting a separate PR with you as a co-author here #248. I realized we needed quite a bit of reworking on the IO types to get systemd happy (which this change was aimed at originally) which made the patches diverge so much that review would be confusing here. Thanks so much for working on this so far!

[elijah-wright]

yea np! I think I learned a lot from this lol. do you want me to close this?

[dcantah]

Yea I can close it out. I couldn't add you as a reviewer over on #248 but please do review if you want to, curious on your insight!