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

not possible to share tty (e.g. usb-dongle) devices with privileged containers #16925

Closed
fho opened this issue Dec 22, 2022 · 13 comments · Fixed by #17055
Closed

not possible to share tty (e.g. usb-dongle) devices with privileged containers #16925

fho opened this issue Dec 22, 2022 · 13 comments · Fixed by #17055
Labels
kind/bug Categorizes issue or PR as related to a bug. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.

Comments

@fho
Copy link
Contributor

fho commented Dec 22, 2022

Is this a BUG REPORT or FEATURE REQUEST? (leave only one on its own line)

/kind bug

Description

I want to run a rootless privileged podman container and access a Sonoff USB zigbee stick in the container.
The container shows up on the host as /dev/ttyACM0.
The host user has access to the device.

In the container the device node is missing.
I found the previous related bugfix #3593.
I believe the issue is that in AddPrivilegedDevices() devices are skipped if their path starts with /dev/tty:

if d.Path == "/dev/ptmx" || strings.HasPrefix(d.Path, "/dev/tty") {
continue
}

Steps to reproduce the issue:

  1. have a tty device to share on the host:
$ ls -la /dev/ttyACM0
crw-rw---- 1 root dialout 166, 0 Dec 22 18:30 /dev/ttyACM0
$ groups
homeassistant dialout
  1. run a privilieged podman container and check if /dev/ttyACM0 is available:
$ podman run  -i -t --privileged  ghcr.io/home-assistant/home-assistant:2022.12.7 ls /dev/tty*

Describe the results you received:
/dev/ttyACM0 device file does not exist in the container

Describe the results you expected:
/dev/ttyACM0 exists in the container

Additional information you deem important (e.g. issue happens only occasionally):

I can share the device via --device if i start it as unprivileged container.

Output of podman version:

podman version 4.3.1

Output of podman info:

N/A

Package info (e.g. output of rpm -q podman or apt list podman or brew info podman):

N/A

Have you tested with the latest version of Podman and have you checked the Podman Troubleshooting Guide?

No, same if-condition exist in the latest commit in main (d20dbcd) branch.

@openshift-ci openshift-ci bot added the kind/bug Categorizes issue or PR as related to a bug. label Dec 22, 2022
@fho fho changed the title rootless privilieged podman container is missing device for usb-dongle (ttyACM0) not possible to share tty (e.g. usb-dongle) devices with privileged containers Dec 22, 2022
@vrothberg
Copy link
Member

Thanks for reaching out, @fho!

@giuseppe do you recall the reasoning for excluding tty?

@giuseppe
Copy link
Member

IIRC they are excluded by default otherwise they could mess up with the ttys on the host

@rhatdan
Copy link
Member

rhatdan commented Dec 23, 2022

I wonder if there is a better way to handle this.

If the user specifies --device /dev/ttyACM0 shouldn't we allow it in?

@fho
Copy link
Contributor Author

fho commented Dec 25, 2022

#15878 is why the check was introduced.

What is if the condition is changed to match only /dev/tty[0-9]+ paths? (as it was also suggested here).

@giuseppe
Copy link
Member

giuseppe commented Jan 3, 2023

that makes sense to me. Would you like to open a PR with that change?

@fho
Copy link
Contributor Author

fho commented Jan 4, 2023

great, yes I can create a PR for it.
I should find some time do it latest on the following weekend (until 15.1).

mupuf added a commit to mupuf/podman that referenced this issue Jan 10, 2023
While mounting virtual terminal devices in a systemd container is a
recipe for disaster (I experienced it first hand), mounting serial
console devices, modems, and others should still be done by default
for privileged systemd-based containers.

This fixes containers#16925.

Fixes: 5a2405a ("Don't mount /dev/tty* inside privileged...")
mupuf added a commit to mupuf/podman that referenced this issue Jan 10, 2023
While mounting virtual terminal devices in a systemd container is a
recipe for disaster (I experienced it first hand), mounting serial
console devices, modems, and others should still be done by default
for privileged systemd-based containers.

Closes containers#16925.

Fixes: 5a2405a ("Don't mount /dev/tty* inside privileged...")
Signed-off-by: Martin Roukala (né Peres) <martin.roukala@mupuf.org>
mupuf added a commit to mupuf/podman that referenced this issue Jan 10, 2023
While mounting virtual terminal devices in a systemd container is a
recipe for disaster (I experienced it first hand), mounting serial
console devices, modems, and others should still be done by default
for privileged systemd-based containers.

Closes containers#16925.

Fixes: 5a2405a ("Don't mount /dev/tty* inside privileged...")
Signed-off-by: Martin Roukala (né Peres) <martin.roukala@mupuf.org>
@mupuf
Copy link
Contributor

mupuf commented Jan 10, 2023

great, yes I can create a PR for it. I should find some time do it latest on the following weekend (until 15.1).

Sorry, I couldn't wait anymore as I had to create a boot2container release without introducing this regression. I hope you had not spent too long on your patch, and if you did, please share it so that we could improve my patch and make it co-developed by you :)

mupuf added a commit to mupuf/podman that referenced this issue Jan 10, 2023
While mounting virtual terminal devices in a systemd container is a
recipe for disaster (I experienced it first hand), mounting serial
console devices, modems, and others should still be done by default
for privileged systemd-based containers.

Closes containers#16925.

Fixes: 5a2405a ("Don't mount /dev/tty* inside privileged...")
Signed-off-by: Martin Roukala (né Peres) <martin.roukala@mupuf.org>
@fho
Copy link
Contributor Author

fho commented Jan 10, 2023

@mupuf I haven't, only ~10min to change the condition and give it a quick test.
I had it still open to investigate if I need to change testcases.
I'm glad that you created the PR, thanks! :-)

@mupuf
Copy link
Contributor

mupuf commented Jan 10, 2023

Great! This makes you a great reviewer and tested for the PR then ;)

mupuf added a commit to mupuf/podman that referenced this issue Jan 10, 2023
While mounting virtual terminal devices in a systemd container is a
recipe for disaster (I experienced it first hand), mounting serial
console devices, modems, and others should still be done by default
for privileged systemd-based containers.

v2, addressing the review from @fho:
 - use backticks in the regular expression to remove backslashes
 - pre-compile the regex at the package level
 - drop IsVirtualTerminalDevice (not needed for a one-liner)

Closes containers#16925.

Fixes: 5a2405a ("Don't mount /dev/tty* inside privileged...")
Signed-off-by: Martin Roukala (né Peres) <martin.roukala@mupuf.org>
mupuf added a commit to mupuf/podman that referenced this issue Jan 10, 2023
While mounting virtual terminal devices in a systemd container is a
recipe for disaster (I experienced it first hand), mounting serial
console devices, modems, and others should still be done by default
for privileged systemd-based containers.

v2, addressing the review from @fho:
 - use backticks in the regular expression to remove backslashes
 - pre-compile the regex at the package level
 - drop IsVirtualTerminalDevice (not needed for a one-liner)

Closes containers#16925.

Fixes: 5a2405a ("Don't mount /dev/tty* inside privileged...")
Signed-off-by: Martin Roukala (né Peres) <martin.roukala@mupuf.org>
mupuf added a commit to mupuf/podman that referenced this issue Jan 10, 2023
While mounting virtual terminal devices in a systemd container is a
recipe for disaster (I experienced it first hand), mounting serial
console devices, modems, and others should still be done by default
for privileged systemd-based containers.

v2, addressing the review from @fho:
 - use backticks in the regular expression to remove backslashes
 - pre-compile the regex at the package level
 - drop IsVirtualTerminalDevice (not needed for a one-liner)

Closes containers#16925.

Fixes: 5a2405a ("Don't mount /dev/tty* inside privileged...")
Signed-off-by: Martin Roukala (né Peres) <martin.roukala@mupuf.org>
mupuf added a commit to mupuf/podman that referenced this issue Jan 11, 2023
While mounting virtual console devices in a systemd container is a
recipe for disaster (I experienced it first hand), mounting serial
console devices, modems, and others should still be done by default
for privileged systemd-based containers.

v2, addressing the review from @fho:
 - use backticks in the regular expression to remove backslashes
 - pre-compile the regex at the package level
 - drop IsVirtualTerminalDevice (not needed for a one-liner)

v3, addressing the review from @fho and @rhatdan:
 - re-introduce a private function for matching the device names
 - use path.Match rather than a regex not to slow down startup time

Closes containers#16925.

Fixes: 5a2405a ("Don't mount /dev/tty* inside privileged...")
Signed-off-by: Martin Roukala (né Peres) <martin.roukala@mupuf.org>
mupuf added a commit to mupuf/podman that referenced this issue Jan 16, 2023
…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 <mail@fholler.de>
Signed-off-by: Martin Roukala (né Peres) <martin.roukala@mupuf.org>

[1]: https://www.cyphar.com/blog/post/20160627-rootless-containers-with-runc
[2]: https://bugzilla.redhat.com/show_bug.cgi?id=501718
mupuf added a commit to mupuf/podman that referenced this issue Jan 16, 2023
…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 <mail@fholler.de>
Signed-off-by: Martin Roukala (né Peres) <martin.roukala@mupuf.org>

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

scottsweb commented Feb 1, 2023

I think I hit this today. Home assistant running priveledged and trying to pass in /dev/ttyUSB0 as a device. The container cannot see it at all.

Any idea when this fix may become available and is there anything I can do to fix it in the meantime?

@mupuf
Copy link
Contributor

mupuf commented Feb 2, 2023

I think I hit this today. Home assistant running priveledged and trying to pass in /dev/ttyUSB0 as a device. The container cannot see it at all.

Any idea when this fix may become available and is there anything I can do to fix it in the meantime?

It should be released as part of podman 4.4... which I assume is about to be released any day now!

@mupuf
Copy link
Contributor

mupuf commented Feb 2, 2023

Well, turns out I should have checked, Podman 4.4 was released 9h ago \o/

https://github.com/containers/podman/releases/tag/v4.4.0

@scottsweb
Copy link

Fantastic. Thanks @mupuf... with any luck that will be an update for Silverblue shortly.

@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 2, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
kind/bug Categorizes issue or PR as related to a bug. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants