Don't mount /dev/ inside privileged containers running systemd #15895
Conversation
|
Please repush your PR. I added [NO NEW TESTS NEEDED] |
openshift-ci
bot
added
the
approved
dcermak
force-pushed
the
don-expose-dev-for-privileged
branch
3 times, most recently
from
78d0925 to
af24c04
Compare
I have removed that and added a bats test, because my initial fix was actually broken (it got late yesterday…). |
dcermak
force-pushed
the
don-expose-dev-for-privileged
branch
2 times, most recently
from
67db995 to
cefd583
Compare
According to https://systemd.io/CONTAINER_INTERFACE/, systemd will try take control over /dev/ttyN if exported, which can cause conflicts with the host's tty in privileged containers. Thus we will not expose these to privileged containers in systemd mode, as this is a bad idea according to systemd's maintainers. Additionally, this commit adds a bats regression test to check that no /dev/ttyN are present in a privileged container in systemd mode This fixes containers#15878 Signed-off-by: Dan Čermák <[email protected]>
dcermak
force-pushed
the
don-expose-dev-for-privileged
branch
from
cefd583 to
5a2405a
Compare
|
@edsantiago Thanks for the review, I've squashed the commits and applied your suggested changes. |
|
|
||
| if [[ $TTYs = "" ]]; then | ||
| die "Did not find any /dev/ttyN devices on local host" | ||
| else |
There was a problem hiding this comment.
It's a little bit confusing to use else after die, since by definition die terminates execution... but not worth re-pushing for, especially given our flaky CI today. I'll fix it in one of my periodic cleanup PRs.
LGTM, but I'll let other more systemd-savvy team members give final approval. Thanks again!
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dcermak, edsantiago, mheon, rhatdan 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 |
Followup to containers#15895: - add a normal-case test, to ensure that --privileged without systemd continues to pass through /dev/ttyN devices - explain why we die() if host has no ttyN devices - I find grep -vx slightly easier to read than sed backslash-slash - run cleanup with '-t 0', to shave ten seconds from CI run Signed-off-by: Ed Santiago <[email protected]>
github-actions
bot
added
the
locked - please file new issue/PR
According to https://systemd.io/CONTAINER_INTERFACE/, systemd will try take control over /dev/tty if exported, which can cause conflicts with the host's tty in privileged containers. Thus we will not expose these to privileged containers in systemd mode, as this is a bad idea according to systemd's maintainers.
This fixes #15878
Does this PR introduce a user-facing change?