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

Add selinux-vs-dmz test case and a workaround #4053

Merged
merged 3 commits into from
Nov 2, 2023

Conversation

kolyshkin
Copy link
Contributor

@kolyshkin kolyshkin commented Oct 4, 2023

Unfortunately, runc-dmz does not play well with SELinux.

The preliminary discussion about that happened in #3987 (comment), and as a result the RUNC_DMZ=legacy was introduced to disable the feature. Alas, the solution is not very practical, as users have to modify upper level runtimes to pass the env var to runc. Also, this is all-or-nothing approach.

The issue itself is described in #4057, and is fixed in container-selinux v2.224.0.

For older distros, this PR adds a workaround (enabled by default). Here's a quote from libct/dmz/README (as added by this PR):

SELinux compatibility issue and a workaround

Older SELinux policy can prevent runc to execute the dmz binary. The issue is
fixed in container-selinux v2.224.0. Yet, some older distributions may not
have the fix, so runc has a runtime workaround of disabling dmz if it finds
that SELinux is in enforced mode and the container SELinux label is set.

Distributions that have a sufficiently new container-selinux can disable the
workaround by building runc with the runc_dmz_selinux_nocompat build flag,
essentially allowing dmz to be used together with SELinux.

@kolyshkin kolyshkin force-pushed the selinux-vs-dmz branch 6 times, most recently from 5b9960c to 60f5203 Compare October 4, 2023 06:59
@lifubang
Copy link
Member

lifubang commented Oct 4, 2023

Adding RUNC_DMZ=legacy helps, but this is not a real solution.

There has a discusstion in here: #3987 (comment), so maybe we should auto detect selinux to disable runc-dmz?

@haircommander
Copy link
Contributor

Adding RUNC_DMZ=legacy helps, but this is not a real solution.

There has a discusstion in here: #3987 (comment), so maybe we should auto detect selinux to disable runc-dmz?

I think we can make the selinux case work, even if it involves changes to https://github.com/containers/container-selinux/

@lifubang
Copy link
Member

lifubang commented Oct 4, 2023

I think we can make the selinux case work

👍 Thanks

@kolyshkin
Copy link
Contributor Author

There has a discusstion in here: #3987 (comment), so maybe we should auto detect selinux to disable runc-dmz?

Something like this, yeah. PR updated.

@kolyshkin kolyshkin force-pushed the selinux-vs-dmz branch 11 times, most recently from dbca1b5 to 34e12ee Compare October 10, 2023 00:05
@kolyshkin kolyshkin changed the title tests/int: add selinux test case [debug] tests/int: add selinux test case Oct 10, 2023
@kolyshkin kolyshkin force-pushed the selinux-vs-dmz branch 4 times, most recently from 59fe2b9 to 4dc8c38 Compare October 11, 2023 01:02
@kolyshkin kolyshkin changed the title [debug] tests/int: add selinux test case Add selinux-vs-dmz test case, runc_dmz_selinux_compat build tag Oct 24, 2023
@kolyshkin kolyshkin marked this pull request as ready for review October 25, 2023 00:25
@kolyshkin kolyshkin added this to the 1.2.0 milestone Oct 25, 2023
Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

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

@kolyshkin
Copy link
Contributor Author

I still prefer a single golden binary that works on any distro (CentOS 7 and later).

If you mean the binary which we publish, sure, we can enable that workaround in there as well (see the last commit).

@kolyshkin kolyshkin force-pushed the selinux-vs-dmz branch 2 times, most recently from cdb537d to 256d89f Compare October 27, 2023 18:19
Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

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

Thanks, the commits LGTM but the PR title and the description are out of sync with the actual commits

@kolyshkin kolyshkin changed the title Add selinux-vs-dmz test case, runc_dmz_selinux_compat build tag Add selinux-vs-dmz test case and a workaround Oct 30, 2023
This is a test case to demonstrate the selinux vs dmz issue.

The issue is, runc calls selinux.SetExecLabel and then execs the
runc-dmz binary, but the execve is denied by selinux:

> type=PROCTITLE msg=audit(10/05/2023 22:54:07.911:10904) : proctitle=/tmp/bats-run-sGk2sn/runc.Ql243q/bundle/runc init
> type=SYSCALL msg=audit(10/05/2023 22:54:07.911:10904) : arch=x86_64 syscall=execveat success=no exit=EACCES(Permission denied) a0=0x6 a1=0xc0000b90fa a2=0xc0000a26a0 a3=0xc000024660 items=0 ppid=105316 pid=105327 auid=root uid=root gid=root euid=root suid=root fsuid=root egid=root sgid=root fsgid=root tty=pts0 ses=8 comm=runc:[2:INIT] exe=/tmp/bats-run-sGk2sn/runc.Ql243q/bundle/runc subj=unconfined_u:unconfined_r:container_runtime_t:s0-s0:c0.c1023 key=(null)
> type=AVC msg=audit(10/05/2023 22:54:07.911:10904) : avc:  denied  { entrypoint } for  pid=105327 comm=runc:[2:INIT] path=/memfd:runc_cloned:runc-dmz (deleted) dev="tmpfs" ino=2341 scontext=system_u:system_r:container_t:s0:c4,c5 tcontext=unconfined_u:object_r:container_runtime_tmpfs_t:s0 tclass=file permissive=0

Once that error is fixed (by adding a selinux rule that enables it), we
see one more error, also related to executing a file on tmpfs.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Add a workaround for a problem of older container-selinux not allowing
runc to use dmz feature. If runc sees that SELinux is in enforced mode
and the container's SELinux label is set, it disables dmz.

Add a build tag, runc_dmz_selinux_nocompat, which disables the workaround.
Newer distros that ship container-selinux >= 2.224.0 (currently CentOS
Stream 8 and 9, RHEL 8 and 9, and Fedora 38+) may build runc with this
build tag set to benefit from dmz working with SELinux.

Document the build tag in the top-level and libct/dmz READMEs.

Use the build tag in our CI builds for CentOS Stream 9 and Fedora 38,
as they already has container-selinux 2.224.0 available in updates.

Add a TODO to use the build tag for CentOS Stream 8 once it has
container-selinux updated.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@kolyshkin
Copy link
Contributor Author

Thanks, the commits LGTM but the PR title and the description are out of sync with the actual commits

fixed

Copy link
Member

@cyphar cyphar left a comment

Choose a reason for hiding this comment

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

LGTM, though I'm not sure that we need runc_dmz_selinux_nocompat either.

@cyphar cyphar merged commit 1f9d9a3 into opencontainers:main Nov 2, 2023
45 checks passed
@kolyshkin
Copy link
Contributor Author

LGTM, though I'm not sure that we need runc_dmz_selinux_nocompat either.

This is for distro packagers -- if they know they have a recent version of container-selinux, they can set this so that their users can enjoy both dmz and selinux.

Once we drop CenOS 7, we can remove this flag (make this behavior a default).

sohankunkerkar added a commit to sohankunkerkar/cri-o that referenced this pull request Nov 6, 2023
Now opencontainers/runc#4053 has fixed,
Let's use the main branch of runc

Signed-off-by: Sohan Kunkerkar <sohank2602@gmail.com>
sohankunkerkar added a commit to sohankunkerkar/cri-o that referenced this pull request Nov 6, 2023
Now opencontainers/runc#4053 has fixed,
Let's use the main branch of runc

Signed-off-by: Sohan Kunkerkar <sohank2602@gmail.com>
sohankunkerkar added a commit to sohankunkerkar/cri-o that referenced this pull request Nov 28, 2023
Now opencontainers/runc#4053 has fixed,
Let's use the main branch of runc

Signed-off-by: Sohan Kunkerkar <sohank2602@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants