Skip to content
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

Make rootless privileged containers share the same tty devices as rootfull ones #17127

Merged
merged 2 commits into from
Jan 17, 2023

Conversation

mupuf
Copy link
Contributor

@mupuf mupuf commented Jan 16, 2023

Until Podman v4.3, privileged rootfull containers would expose all the
host devices to the container while rootless ones would exclude
`/dev/ptmx` and `/dev/tty*`.

When 5a2405ae1b3a ("Don't mount /dev/tty* inside privileged containers
running systemd") landed, rootfull containers started excluding all the
`/dev/tty*` devices when the container would be running in systemd
mode, reducing the disparity between rootless and rootfull containers
when running in this mode.

However, this commit regressed some legitimate use cases: exposing
non-virtual-terminal tty devices (modems, arduinos, serial
consoles, ...) to the container, and the regression was addressed in
f4c81b0aa5fd ("Only prevent VTs to be mounted inside privileged
systemd containers").

This now calls into question why all tty devices were historically
prevented from being shared to the rootless non-privileged containers.
A look at the podman git history reveals that the code was introduced
as part of ba430bfe5ef6 ("podman v2 remove bloat v2"), and obviously
was copy-pasted from some other code I couldn't find.

In any case, we can easily guess that this check was put for the same
reason 5a2405ae1b3a was introduced: to prevent breaking the host
environment's consoles. This also means that excluding *all* tty
devices is overbearing, and should instead be limited to just virtual
terminals like we do on the rootfull path.

This is what this commit does, thus making the rootless codepath behave
like the rootfull one when in systemd mode.

This leaves `/dev/ptmx` as the main difference between the two
codepath. Based on the blog post from the then-runC maintainer[1] and
this Red Hat bug[2], I believe that this is intentional and a needed
difference for the rootless path.

Closes: #16925
Suggested-by: Fabian Holler <[email protected]>
Signed-off-by: Martin Roukala (né Peres) <[email protected]>

[1]: https://www.cyphar.com/blog/post/20160627-rootless-containers-with-runc
[2]: https://bugzilla.redhat.com/show_bug.cgi?id=501718

Does this PR introduce a user-facing change?

Rootless privileged containers will now mount all tty devices, except for the virtual-console ones (/dev/tty[0-9]+)

@mupuf
Copy link
Contributor Author

mupuf commented Jan 16, 2023

@fho: I hope this wall of text makes sense!

mupuf added 2 commits January 16, 2023 16:23
…tfull ones

Until Podman v4.3, privileged rootfull containers would expose all the
host devices to the container while rootless ones would exclude
`/dev/ptmx` and `/dev/tty*`.

When 5a2405a ("Don't mount /dev/tty* inside privileged containers
running systemd") landed, rootfull containers started excluding all the
`/dev/tty*` devices when the container would be running in systemd
mode, reducing the disparity between rootless and rootfull containers
when running in this mode.

However, this commit regressed some legitimate use cases: exposing
non-virtual-terminal tty devices (modems, arduinos, serial
consoles, ...) to the container, and the regression was addressed in
f4c81b0 ("Only prevent VTs to be mounted inside privileged
systemd containers").

This now calls into question why all tty devices were historically
prevented from being shared to the rootless non-privileged containers.
A look at the podman git history reveals that the code was introduced
as part of ba430bf ("podman v2 remove bloat v2"), and obviously
was copy-pasted from some other code I couldn't find.

In any case, we can easily guess that this check was put for the same
reason 5a2405a was introduced: to prevent breaking the host
environment's consoles. This also means that excluding *all* tty
devices is overbearing, and should instead be limited to just virtual
terminals like we do on the rootfull path.

This is what this commit does, thus making the rootless codepath behave
like the rootfull one when in systemd mode.

This leaves `/dev/ptmx` as the main difference between the two
codepath. Based on the blog post from the then-runC maintainer[1] and
this Red Hat bug[2], I believe that this is intentional and a needed
difference for the rootless path.

Closes: containers#16925
Suggested-by: Fabian Holler <[email protected]>
Signed-off-by: Martin Roukala (né Peres) <[email protected]>

[1]: https://www.cyphar.com/blog/post/20160627-rootless-containers-with-runc
[2]: https://bugzilla.redhat.com/show_bug.cgi?id=501718
Users need to know about this side effect.

Fixes: 5a2405a ("Don't mount /dev/tty* inside privileged...")
Fixes: f4c81b0 ("Only prevent VTs to be mounted inside ...")
Signed-off-by: Martin Roukala (né Peres) <[email protected]>
@mupuf mupuf force-pushed the tty_devices_for_all branch from 409e842 to 8db2b4b Compare January 16, 2023 14:24
@mupuf
Copy link
Contributor Author

mupuf commented Jan 16, 2023

2 test failures seem to be unrelated to my changes (502 from docker.io in the int * tests), and no idea why the APIv2 tests failed :s

@fho
Copy link
Contributor

fho commented Jan 16, 2023

@mupuf sounds good to me, great explanation and references!

@mheon
Copy link
Member

mheon commented Jan 16, 2023

LGTM

@edsantiago
Copy link
Member

Submitted containers/buildah#4521 to try to fix the flake, but that's not going to help this PR. ITM all we can do is play the rerun game.

Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 17, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jan 17, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fho, mupuf, vrothberg

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 17, 2023
@openshift-merge-robot openshift-merge-robot merged commit abfca47 into containers:main Jan 17, 2023
@giuseppe
Copy link
Member

this caused a regression with runc:

$ bin/podman --runtime /usr/bin/runc run --rm  --privileged alpine true
Error: OCI runtime error: /usr/bin/runc: runc create failed: unable to start container process: error during container init: error creating device nodes: open ~/.local/share/containers/storage/overlay/9c71e9c49cc103f6d02c7a2a2cbb81394f7fb109fcfd5034320c235825dd1c61/merged/dev/tty: no such device or address

@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 12, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants