Clarify wording for terminal setting and /dev/console

[mrunalp]

Signed-off-by: Mrunal Patel mrunalp@gmail.com

[crosbymichael]

LGTM

[wking]

I think this is still too vague. Who allocates the terminal? It sounds like runC expects the runtime-caller to already have a slave, although ccon uses this setting to have the container process opens the pair with posix_openpt. See also #494.

[mrunalp]

@wking I think that both approaches are valid and we shouldn't add language to support one but not the other.

[wking]

On Wed, Jul 20, 2016 at 12:10:26PM -0700, Mrunal Patel wrote:

-* terminal (bool, optional) specifies whether you want a terminal attached to that process. Defaults to false.
+* terminal (bool, optional) specifies whether a terminal is allocated for and attached to that process. Defaults to false.

@wking I think that both approaches are valid and we shouldn't add
language to support one but not the other.

I'm ok with being vague in config.md as long as it gets spelled out in
the command-line API specs. Otherwise this is going to be hard to
compliance-test. If we're trying to focus just on the
inside-the-container view here, how about:

If true, the container process's standard streams will all be
attached to a terminal. Defaults to false.

If we're not going into detail about who allocates the terminal, I'd
rather not mention “allocate” at all.

[wking]

I didn't record an entry for this in the meeting log, but I remember
saying (around 1?) that I'd post notes here to a ccon implementation
of opening the psuedoterminal pair inside the container (which ccon
has done for a while) and passing it out to the caller via a Unix
socket (ccon previously only passed master from its container process
to its host process). I've just pushed an implementaiton in 2,
where:

$ ccon … --pseudoterminal-socket PATH

will connect to the Unix socket listening at PATH, and pass the master
file descriptor through. The current implementation isn't fully
polished:

• The message should probably carry both the master file descriptor
  and an identifying token (e.g. the container ID) so that a single
  listener can collect masters from multiple containers.
• There's currently no way to have a ccon invocation exit early. The
  host process waits for the container process to collect and return
  its exit code, regardless of whether it passes the master file
  descriptor out or not.

But it should be sufficient for folks who want to kick the tires of
the master pass-out approach.

[mrunalp]

Rebased and updated.

[wking]

For extra clarity (and to match “Atomically duplicate 'oldfd' on 'newfd'” wording in dup(2) and “pseudoterminal” in pts(4)), I'd recommend:

On Linux, a psuedoterminal pair is allocated for the container process and the psuedoterminal slave is duplicated on the container process's standard streams.

[mrunalp]

Updated.

[wking]

I think “bind mounted” is overly specific. Ccon does not bind mount /dev/console, it relies on the container's posix_openpt and the kernel/libc implementation to get the appropriate console device behavior. In this case, I suggest we lead with the ocitools implementation of the test (how do you validate proper /dev/console handling?) and then figure out the wording after we have ocitools tests we like. I expect the test will be something like “container writes to /dev/console and test harness ensures it reads that string out of the pseudoterminal master”, but we may want other tests in addition (I'm not familiar enough with /dev/console to know).

[wking]

Actually, /dev/console looks like a different beast from the psuedoterminal. Why link them at all?

[hqhq]

I think /dev/console can be a common terminal and not just pseudoterminal, no?

[mrunalp]

@hqhq For container runtimes, it is the pseudoterminal slave. I haven't seen a use case or setup otherwise.

[mrunalp]

This is ready for review. ping @opencontainers/runtime-spec-maintainers

[wking]

I still think:

• We want /dev/console unconditionally.
• We don't want to require bind-mounting the slave over this. Let the runtime point /dev/console to a place it considers appropriate for logging system messages, which may not be the pseudoterminal slave. In fact, I'd expect the validation for /dev/console to be “Is the major 5, the minor 1, and the type ‘char’?”, which is not going to be satisfied by bind-mounting the slave.

[cyphar]

We don't want to require bind-mounting the slave over this. Let the runtime point /dev/console to a place it considers appropriate for logging system messages, which may not be the pseudoterminal slave. In fact, I'd expect the validation for /dev/console to be “Is the major 5, the minor 1, and the type ‘char’?”, which is not going to be satisfied by bind-mounting the slave.

That validation does not work inside containers. That particular major and minor number is reserved for consoles that are created as part of initial bootup (think physical console) on Linux. I think that it is entirely sufficient to just state that /dev/console should be a valid place to log system messages (which is what you suggested).

[hqhq]

@cyphar According to the new console rewrite in runc, any thoughts about how should we define /dev/console in runtime-spec? I'm afraid bind mount pseudoterminal slave to /dev/console would be too implementation detailed for spec, and it should not be the only way for a container.

[wking]

On Thu, Sep 29, 2016 at 01:49:00AM -0700, Aleksa Sarai wrote:

-* [/dev/console][console.4]
+* [/dev/console][console.4] is setup if terminal is enabled in the config by bind mounting the pseudoterminal slave to /dev/console.

We don't want to require bind-mounting the slave over this. Let
the runtime point /dev/console to a place it considers appropriate
for logging system messages, which may not be the pseudoterminal
slave. In fact, I'd expect the validation for /dev/console to be
“Is the major 5, the minor 1, and the type ‘char’?”, which is not
going to be satisfied by bind-mounting the slave.

That validation does not work inside containers. That particular
major and minor number is reserved for consoles that are created as
part of initial bootup (think physical console) on Linux.

It can work inside containers:

$ unshare -Urm file /dev/console
/dev/console: character special (5/1)

All of the logged messages will end up in the host console, but maybe
that's the semantics we want.

I think that it is entirely sufficient to just state that
/dev/console should be a valid place to log system messages (which
is what you suggested).

If we allow devices other than c5/1 at that path, I think we need to
be very clear about the API they should support. For example, screen
uses TIOCCONS to redirect the console output. Is that required to
work inside OCI containers (assuming the caller has sufficient
permission 1)? You can test this with ‘ctrl-a :console on’ in a
screen. Does that catch output written to /dev/console if it isn't
c5/1?

[cyphar]

@wking

Sure, that validation /could/ work. But what you've done in your example is just use the parent's /dev/console. If we just did mknod("/dev/console", makedev(5,1)) then a container could start modifying what the host console sees (read: the console that people trying to administer the system are going to log into). So we definitely do not want to make that a restriction (which is what I meant when I said "validation"), because it could be seen as a security issue. If you want to have any restriction for what we support then we should simply say that it must be "a console" (which on Linux means that it's the slave of a ptmx pair).

[hqhq]

Yeah I think /sys/dev/char/5:1/ is not isolated, if you mknod /dev/console with c(5, 1) in the container and change the backend device, it'll change /sys/dev/char/5:1/ and affect host.

[wking]

On Thu, Sep 29, 2016 at 08:15:03PM -0700, Qiang Huang wrote:

Yeah I think /sys/dev/char/5:1/ is not isolated, if you mknod
/dev/console with c(5, 1) in the container and change the backend
device, it'll change /sys/dev/char/5:1/ and affect host.

I'm fine with that (although I'd also be fine with a console device
that was namespace-able like devpts is). If you don't want your
containers redirecting /dev/console, don't give them CAP_SYS_ADMIN.
And if you don't want your containers writing to /dev/console, adjust
its permissions so they can't touch it (which may include mapping
containers from non-root host users). If you launch containers from
root host users and let them keep CAP_SYS_ADMIN, then presumably
you're ok with them mucking about with the host /dev/console (maybe
you have a containerized console monitor?).

If you don't care about TIOCCONS you can mount another pseudoterminal
slave (or whatever) at /dev/console, and tools that just write to
/dev/console won't care. But you can do that with explicit settings
in your container config. Of course, you can create a c5:1 device
with explicit settings in the container config too. It's unclear to
me if an entry in linux.devices will clobber or error if the target
path already exists (e.g. because the runtime has bind-mounted the
pseudoterminal slave there). It's also unclear to me if a the
pseudoterminal is opened early enough for a mounts entry to bind-mount
/dev/pts/1 over /dev/console. A pre-start hook or post-create
manipulation would work for either approach, regardless of what the
runtime sets up during the create call. So whichever way we pick, we
should be very clear about it, so runtime callers can take steps to
get their desired /dev/console if it doesn't match what the spec
requires the runtime to create.

[cyphar]

@wking Again, I never said that using /dev/console should be forbidden (though I think such use would be A Bad Idea (tm)). What I'm saying is that we must not restrict /dev/console to always be 5:1 because of the issues I've discussed.

I also maintain that "somewhere to dump system messages" is sufficient restriction / definition for users. If someone wants to use something specialised like TIOCCONS modify it then they should use the tools we provide to do so -- it doesn't make sense to cripple the spec to pander to people who want to use a very odd (and host-centric so it doesn't even make much sense in the context of containers) API.

[wking]

On Fri, Sep 30, 2016 at 04:04:14AM -0700, Aleksa Sarai wrote:

I also maintain that "somewhere to dump system messages" is
sufficient restriction / definition for users. If someone wants to
use something specialised like TIOCCONS modify it then they should
use the tools we provide to do so…

So the spec entry should be something like:

• [/dev/console][console.4] MUST be a terminal and MAY be the system console.
  Only the system console supports TIOCCONS, so containers relying on TIOCCONS SHOULD check the runtime-provided /dev/console and if neccessary replace it with a [system console][] device.

[cyphar]

@wking I think this is what we should put:

* [`/dev/console`][console.4] MUST be the preferred location for system
  messages to be written by the container (aside from the standard streams). It
  SHOULD be a terminal and MAY be the [system console][system-console]. Only the
  system console supports [`TIOCCONS`][tty_ioctl.4], so containers relying on
  `TIOCCONS` SHOULD check the runtime-provided `/dev/console` and if neccessary
  replace it with a [system console][] device.

Footnotes the same as your comment. I'm not sure about the terminal restriction to be completely honest. But I'd consider using your wording if you can give a good example of how a misreading of the spec could result in something we shouldn't allow happening.

[wking]

On Wed, Oct 05, 2016 at 12:59:48PM -0700, Qiang Huang wrote:

Is TIOCCONS the only case that requires /dev/console to be a
system console?

I am not aware of any others, but I like your more conservative
wording better.

[wking]

In today's meeting there was more discussion on whether /dev/console
should be required when process.terminal isn't true 1. I argued for
always requiring /dev/console, since it's for system logs, and that
seems orthogonal to process.terminal. @crosbymichael pointed out
resource-consumption issues if the runtime opens a new pseudoterminal
pair for each container (if it tries to source that /dev/console by
bind-mounting a pseudoterminal slave). And he pointed out that runC
currently only creates /dev/console when process.terminal is true and
that works pretty well. I suggested not requiring /dev/console at all
then (and users who need it can add a devices entry for c5/1 or
bind-mount their stdin over /dev/console as they see fit), and
@crosbymichael cited past concerns with tmux and sshd. However, I see
no references to /dev/console in either codebase [2,3]. The only code
I've been able to find that actually uses /dev/console is screen, and
then only for TIOCCONS [4,5]. If we are going to require /dev/console
at all, I think we should either always require it (regardless of
process.terminal) or build a better case for why it's related to
process.terminal. For example, with explict steps to generate
/dev/console-is-missing errors in tmux or OpenSSH, or links to issues
that describe those steps.

[cyphar]

@wking

I suggested not requiring /dev/console at all then (and users who need it can add a devices entry for c5/1 or bind-mount their stdin over /dev/console as they see fit), and @crosbymichael cited past concerns with tmux and sshd. However, I see no references to /dev/console in either codebase [2,3].

If you're willing to test a VERY large amount of software with that change, then I would accept removing the requirement. However, that would just be time consuming and i would prefer that we have the requirement and then drop it in later versions of the spec. This PR (and all of the other PRs for terminal) have dragged on for months and months and it's starting to get quite aggravating TBH.

While it might be true that /dev/console is not required I would be very careful about removing things like that without testing a very large number of container setups. Consider that the runC console changes actually don't change any functionality and how cautious the other maintainers are about merging it (they're testing everything before accepting it). That is the level of caution we should have about removing these features.

[wking]

On Tue, Oct 18, 2016 at 01:51:25AM -0700, Aleksa Sarai wrote:

I suggested not requiring /dev/console at all then (and users who
need it can add a devices entry for c5/1 or bind-mount their stdin
over /dev/console as they see fit), and @crosbymichael cited past
concerns with tmux and sshd. However, I see no references to
/dev/console in either codebase [2,3].

If you're willing to test a VERY large amount of software with
that change, then I would accept removing the requirement. However,
that would just be time consuming and i would prefer that we have
the requirement and then drop it in later versions of the spec. This
PR (and all of the other PRs for terminal) have dragged on for
months and months and it's starting to get quite aggravating TBH.

I think that “provide something we're not sure we need, since we can
remove it later” is going the wrong way ;). Can we not provide it
until we're sure we need it? I think the burden of proof for a
feature should be to show why it's needed (which you can actually do),
not to show that nobody needs it (which seems impossible). That said,
if you have suggestions on what you'd like to see for demonstrating
that it's not needed, I'm willing to give it a go.

While we're working out when /dev/console is actually needed, folks
who know they need it (because they're planning on using TIOCCONS?) or
suspect they need it can easily add a linux.devices entry or do
post-create bind-mounting.

[cyphar]

@wking

I think that “provide something we're not sure we need, since we can remove it later” is going the wrong way ;). Can we not provide it until we're sure we need it?

Wouldn't that be non-backwards-compatible? Since we'd be adding a new restriction to runtimes? Or would that be fine for 1.1?

I think the burden of proof for a feature should be to show why it's needed (which you can actually do), not to show that nobody needs it (which seems impossible).

/dev/console was added to Docker/runC for reasons of "legacy software". The status quo for containers is that it exists -- so the burden of proof is on you (that it is no longer a concern).

That said, if you have suggestions on what you'd like to see for demonstrating that it's not needed, I'm willing to give it a go.

Remove the /dev/console handling from runC (and Docker) and run as many different images using Docker to see whether or not everything works in both instances. I can't give you a list of software to test, because I'm not sure what software uses /dev/console. Maybe some GitHub searching can help you here.

While we're working out when /dev/console is actually needed, folks who know they need it (because they're planning on using TIOCCONS?) or suspect they need it can easily add a linux.devices entry or do post-create bind-mounting.

Again, c5/1 is not going to be a requirement for /dev/console. I will NACK any patch that makes this a requirement (for the reasons myself and several other people outlined above). So if someone wants to use c5/1 they will have to either check their runtime or explicitly make it themselves. For the "general logging" usecase, that's what we're testing here.

[wking]

On Tue, Oct 18, 2016 at 03:43:25AM -0700, Aleksa Sarai wrote:

I think that “provide something we're not sure we need, since we
can remove it later” is going the wrong way ;). Can we not
provide it until we're sure we need it?

Wouldn't that be non-backwards-compatible? Since we'd be adding a
new restriction to runtimes? Or would that be fine for 1.1?

Both major and minor bumps add new restrictions to runtimes. The
backwards compatibility is for configs (so a 1.0 config still works
with a 1.1 runtime). The wording for this landed in #523.

I think the burden of proof for a feature should be to show why
it's needed (which you can actually do), not to show that nobody
needs it (which seems impossible).

/dev/console was added to Docker/runC for reasons of "legacy
software". The status quo for containers is that it exists -- so the
burden of proof is on you (that it is no longer a concern).

“Somebody else has already picked this direction” isn't a good
argument. If a solid argument around legacy software needing
/dev/console mostly when process.terminal is true exists in the Docker
/ runC archives, then folks who are pushing for runtime-spec to follow
that example should be able to link to that previous discussion.
Then it would be on me to try and poke holes in anything I didn't
find convincing. But saying that the discussion happened without
giving a reference to it makes it hard to have a rational discussion
here.

That said, if you have suggestions on what you'd like to see for
demonstrating that it's not needed, I'm willing to give it a go.

Remove the /dev/console handling from runC (and Docker) and run as
many different images using Docker to see whether or not everything
works in both instances. I can't give you a list of software to
test, because I'm not sure what software uses /dev/console. Maybe
some GitHub searching can help you here.

GitHub searching turns up many, many links to kernel docs ;). I
didn't get deep enough into the results to find any non-kernel
software using it. I'll start translating Docker images to OCI and
see if they run without /dev/console, although “see whether or not
everything works” sounds like an impossibly high bar, even for a
container as simple as a SSH daemon process. There are too many
possibilities to handle as a black box without grepping around for
/dev/console.

While we're working out when /dev/console is actually needed,
folks who know they need it (because they're planning on using
TIOCCONS?) or suspect they need it can easily add a linux.devices
entry or do post-create bind-mounting.

Again, c5/1 is not going to be a requirement for /dev/console.

Agreed. Mounting c5/1 is just one of several options for providing
/dev/console. The post-create bind-mounting is for folks who want to
bind /proc/${CONTAINER_PID}/fd/1 over /dev/console.

[wking]

Poking around trying to find /dev/console consumers turned up syslog
(unsurprisingly, since /dev/console is for system logging). syslog-ng
doesn't hard-code /dev/console anywhere, but there are commented-out
entries in their Fedora [1,2] and Red Hat [3,4] examples. So folks
running syslog in their containers who have configured it to write to
/dev/console will need something terminal-like there.

rsyslogd has _PATH_CONSOLE defaulting to /dev/console (at
compile-time) 5. It is only used for TTY “detection” 6. There is
also a commented-out entry in the RedHat example config 7. So this
seems like the same situation as syslog-ng: the logger doesn't hit
/dev/console by default, but a user might configure it to write there.

The “syslog may be configured to write to /dev/console” seems
orthogonal to process.terminal to me. And if we don't require
runtimes to supply /dev/console at all, those users can either
configure syslog to write somewhere else or use linux.devices or
post-create manipulation to put a terminal at /dev/console.

[crosbymichael]

systemd uses it

systemd/systemd#4262

[mrunalp]

Rebased.

[wking]

On Wed, Oct 19, 2016 at 10:09AM -0700, Michael Crosby wrote:

systemd/systemd#4262

Ah, thanks :). In that issue, Lennart links to 1 which has:

If you write a container solution, please consider supporting the
following interfaces.

Execution Environment

…

1. Pre-mount /dev as (container private) tmpfs for the container and
   bind mount some suitable TTY to /dev/console. Also, make sure to
   create device nodes for /dev/null, /dev/zero, /dev/full,
   /dev/random, /dev/urandom, /dev/tty, /dev/ptmx in /dev…

   …

2. If systemd detects it is run in a container it will spawn a
   single shell on /dev/console, and not care about VTs or multiple
   gettys on VTs. (But see $container_ttys below.)

   …

   Environment Variables

   …

3. systemd can automatically spawn login gettys on additional
   ptys. A container manager can set the $container_ttys=
   environment variable for the container's PID 1 to tell it on
   which ptys to spawn gettys. The variable should take a space
   seperated list of pty names, without the leading "/dev" prefix,
   but with the "pts/" prefix included. Note that despite the
   variable's name you may only specify ptys, and not other types of
   ttys. Also you need to specify the pty itself, a symlink will not
   suffice. This is implemented in systemd-getty-generator(8). Note
   that this variable should not include the pty that /dev/console
   maps to if it maps to one (see below). Example: if the container
   receives "container_ttys=pts/7 pts/8 pts/14" it will spawn three
   additionally login gettys on ptys 7, 8 and 14.

   …

   What You Shouldn't Do

   …

4. Do not cross-link /dev/kmsg with /dev/console. They are different
   things, you cannot link them to each other.

The “single shell on /dev/console” bit is similar to the kernel's “on
which logins should be permitted in single-user mode” 2. This usage
also couples /dev/console to process.terminal. I don't think that
coupling is strong enough to lean on it here though, because the
syslog use-case is not coupled to terminal stdio for the container
process (when configured to use /dev/console, syslog would be
explicitly opening it for logging). I'm not familiar enough with
systemd, and there are too many /dev/console hits in its code for me
to find out quickly, but it's possible that you'd want to run systemd
with or without terminal stdio too.

So I recommend we add a new console boolean (linux.console? Maybe
also solaris.console?) to tell the runtime whether or not it needs to
provide /dev/console. The runtime would be free to provide that
/dev/console however it likes, as long as it provides a terminal.
runC would probably be:

• Creating a single pseudoterminal pair if either process.terminal or
  linux.console was set.
• Exposing the slave at /dev/console if linux.console was set (via a
  bind mount).
• Duping the slave onto the container process's stdio if
  process.terminal was set.

Does that sound workable?

[cyphar]

@wking You mentioned in the weekly call that you'd put a summary of the discussion here? Would you mind doing so?

[wking]

On Fri, Oct 21, 2016 at 10:19:14AM -0700, Aleksa Sarai wrote:

@wking You mentioned in the weekly call that you'd put a summary of the discussion here? Would you mind doing so?

Sorry, GitHub seems to be eating my emailed comments. Posted summary was for #513, and I just re-posted via the web-UI here.

[tianon]

@cyphar is the wording in this PR accurate in your opinion, or should it be adjusted?

IMO it sounds like it's an improvement over current master as-is

[cyphar]

Yeah, this is overall an improvement. This overall LGTM, we can deal with whether we should remove some of the restrictions later.

[tianon]

LGTM

Thanks for confirming! ❤️

[hqhq]

LGTM