Confined user fixes and bump to v2.247.0#443
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideUpdates SELinux container policy and main test plan configuration to support new confined user behavior and align everything with version v2.247.0. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Summary of ChangesHello @lsm5, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request primarily enhances SELinux policies to improve the functionality for confined users when interacting with container runtimes, addressing permissions for process signaling and socket management crucial for user-level systemd services. Additionally, it refines the execution logic for a specific test, making it conditional based on the triggering mechanism. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
lsm5
force-pushed
the
confined-user-fixes
branch
from
a397e57 to
e58b3fc
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request primarily focuses on enhancing SELinux policy to properly support confined users, allowing them to manage processes and sockets necessary for podman.socket activation. The changes in container.te appear to be well-targeted and essential for the described functionality. Additionally, the FMF test plan has been updated to conditionally enable xmllint_validation only for packit initiated runs. While the SELinux policy updates are clear, the change in the test plan could potentially reduce the overall validation coverage for XML files in non-Packit testing environments.
| enabled: false | ||
| adjust: | ||
| - when: initiator == packit | ||
| enabled: true |
There was a problem hiding this comment.
Disabling the xmllint_validation test by default and only enabling it for packit runs might lead to reduced validation coverage in other testing environments. If this test is crucial for maintaining the integrity of XML files, it should ideally run in all relevant contexts. Consider if the test can be made more robust or efficient to run universally, or if there's a specific reason it's problematic outside of packit.
If the test is consistently failing or causing issues, it might be better to address the root cause rather than conditionally disabling it, as this could mask potential problems in other CI pipelines or local development environments.
There was a problem hiding this comment.
Testing upstream suffices for now. I will need to change some of the propose-downstream Packit job setup if we want to fetch the entire upstream source tree for downstream tests. Ignoring this for now.
|
Ephemeral COPR build failed. @containers/packit-build please check. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
|
@containers/container-selinux-maintainers PTAL. |
|
The issue here is transitioning from a user_r to a system_r and a user_t to a container_runtime_t, are pretty liberal changes. container_runtime_t is a unconfined domain, and system_r are roles usually assigned to a more powerful confined users. |
@rhatdan ack, thanks. Would wrapping this behind a new boolean be acceptable? |
|
Sure confined_user_run_containers or something. |
lsm5
force-pushed
the
confined-user-fixes
branch
from
e78377b to
48e89ba
Compare
|
/packit rebuild-failed |
|
COPR infra is down. Will retry in a bit. |
|
@rhatdan PTAL. Ignore the CI issues for now. |
container.te
Outdated
| # Allow confined user systemd instances to create and manage sockets | ||
| # for podman.socket activation (user-level systemd pre-labels the | ||
| # socket as container_runtime_t via setsockcreatecon) | ||
| allow staff_t container_runtime_t:unix_stream_socket { create bind listen getattr setattr setopt shutdown }; |
There was a problem hiding this comment.
That's maybe stupid question, but do we really need setattr and shutdown?
There was a problem hiding this comment.
@jankaluza ack. I can try without those.
@rhatdan btw, I see many occurrences of the create_stream_socket_perms macro which includes a bunch of perms. Are we ok to use it or should we consider fine-tuning / perhaps creating a separate macro?
lsm5
force-pushed
the
confined-user-fixes
branch
from
48e89ba to
f908d02
Compare
|
Please hold off on further reviews for now. I need to test this further. |
lsm5
force-pushed
the
confined-user-fixes
branch
from
f908d02 to
f1ee8d5
Compare
|
Tests failed. @containers/packit-build please check. |
lsm5
force-pushed
the
confined-user-fixes
branch
2 times, most recently
from
eeac906 to
adfe197
Compare
|
@containers/container-selinux-maintainers PTAL, good for another look. The |
When confined users run "systemctl --user start podman.socket", their
user-level systemd instance needs to create a unix stream socket
pre-labeled as container_runtime_t (via setsockcreatecon). No policy
rule existed to permit this, causing an AVC denial on the socket
create syscall.
Grant staff_t and user_t the necessary unix_stream_socket permissions
on container_runtime_t so that systemd socket activation of podman
works for both confined user types.
Fixes the following AVCs:
staff_t:
```
type=PROCTITLE msg=audit(1764134806.202:243): proctitle="(systemd)"
type=SYSCALL msg=audit(1764134806.202:243): arch=c000003e syscall=41 success=no exit=-13
a0=1 a1=80801 a2=0 a3=0 items=0 ppid=1 pid=943 auid=1000 uid=1000 gid=1000 euid=1000
suid=1000 fsuid=1000 egid=1000 sgid=1000 fsgid=1000 tty=(none) ses=1
comm="systemd" exe="/usr/lib/systemd/systemd" subj=staff_u:staff_r:staff_t:s0-s0:c0.c1023 key=(null)
type=AVC msg=audit(1764134806.202:243): avc: denied
{ create } for pid=943 comm="systemd" scontext=staff_u:staff_r:staff_t:s0-s0:c0.c1023
tcontext=staff_u:staff_r:container_runtime_t:s0-s0:c0.c1023 tclass=unix_stream_socket permissive=0
```
user_t:
```
type=PROCTITLE msg=audit(1772104645.324:301): proctitle="(systemd)"
type=SYSCALL msg=audit(1772104645.324:301): arch=c000003e syscall=41 success=no exit=-13
a0=1 a1=80801 a2=0 a3=0 items=0 ppid=1 pid=1649 auid=1001 uid=1001 gid=1001 euid=1001
suid=1001 fsuid=1001 egid=1001 sgid=1001 fsgid=1001 tty=(none) ses=11
comm="systemd" exe="/usr/lib/systemd/systemd" subj=user_u:user_r:user_t:s0 key=(null)
type=AVC msg=audit(1772104645.324:301): avc: denied
{ create } for pid=1649 comm="systemd" scontext=user_u:user_r:user_t:s0
tcontext=user_u:user_r:container_runtime_t:s0 tclass=unix_stream_socket permissive=0
```
Fixes: RHEL-132875, RHEL-135340
Signed-off-by: Lokesh Mandvekar <[email protected]>
Add unprivuser_role_change_to(system_r) so user_r can transition to
system_r for container processes. Replace container_runtime_run(user_t,
user_r) with its RBAC components (role statements) kept unconditional
and move container_runtime_domtrans(user_t) plus signal_perms into a
new tunable_policy block gated by user_t_run_containers (default: off).
RBAC rules cannot be placed inside tunable_policy blocks, so the role
statements and unprivuser_role_change_to(system_r) remain unconditional.
The domain transition from user_t to container_runtime_t is only
permitted when the boolean is enabled.
Fixes the following error and AVC:
```
$ id -Z
user_u:user_r:user_t:s0
$ podman run --rm -it ubi9
exec /bin/bash: permission denied
type=PROCTITLE msg=audit(1772104167.258:882):
proctitle=2F7573722F62696E2F72756E6300696E6974
type=SYSCALL msg=audit(1772104167.258:882): arch=c000003e syscall=59 success=no
exit=-13 a0=c0002d1ae0 a1=c00002d5e0 a2=c0002153b0 a3=0 items=0 ppid=7581 pid=7593
auid=1001 uid=1001 gid=1001 euid=1001 suid=1001 fsuid=1001 egid=1001 sgid=1001
fsgid=1001 tty=pts0 ses=16 comm="runc:[2:INIT]" exe="/runc"
subj=user_u:user_r:container_runtime_t:s0 key=(null)
type=AVC msg=audit(1772104167.258:882): avc: denied { transition } for
pid=7593 comm="runc:[2:INIT]" path="/usr/bin/bash" dev="overlay" ino=411042085
scontext=user_u:user_r:container_runtime_t:s0
tcontext=system_u:system_r:container_t:s0:c273,c965 tclass=process permissive=0
```
Fixes: RHEL-135342
Signed-off-by: Lokesh Mandvekar <[email protected]>
The xmllint test requires container-selinux source cloned which is currently only done in upstream CI environments. Disable it by default and enable it only when the initiator is packit. Signed-off-by: Lokesh Mandvekar <[email protected]>
Signed-off-by: Lokesh Mandvekar <[email protected]>
lsm5
force-pushed
the
confined-user-fixes
branch
from
adfe197 to
9c8eb84
Compare
|
rebased on the latest from main. No changes. |
4 participants
See individual commits.
Summary by Sourcery
Update SELinux container policy and main plan configuration for confined user support and bump the project version to v2.247.0.
Bug Fixes:
Enhancements:
Build: