-
Notifications
You must be signed in to change notification settings - Fork 203
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
Allow rootless containers to use AppArmor profiles #960
Allow rootless containers to use AppArmor profiles #960
Conversation
f908d56
to
8d6b81c
Compare
Built with these changes to
The check for whether the profile exists gets deferred to the container runtime when it tries to write to The downside of all this is that you get a somewhat opaque error if you try to run a container with a nonexistent profile:
But that's an issue for
(Which is inaccurate since AppArmor is in fact enabled on my system.) |
Previously, Podman would print an error if you tried to run a container with an AppArmor profile as a non-root user, e.g. $ podman run --security-opt apparmor=my-profile ... Error: Apparmor profile "my-profile" specified, but Apparmor is not enabled on this system In fact, the only thing that Podman needs root privileges for is reading /sys/kernel/security/apparmor/profiles to see if the profile is already loaded, which isn't strictly necessary. This commit removes the 'IsLoaded()' check that occurs when you try to specify an AppArmor profile as a non-root user, as well as the other checks in pkg/apparmor/ for whether the program is running as UID 0. The check for whether the AppArmor profile is loaded should now be deferred to the container runtime at the point where it writes to either /proc/self/attr/exec or /proc/self/attr/apparmor/exec, since the write should fail if the profile is not loaded. Closes containers#958. Signed-off-by: kernelmethod <[email protected]>
8d6b81c
to
55d217f
Compare
LGTM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
@giuseppe, can you give a final head nod?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: giuseppe, kernelmethod, saschagrunert 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 |
/lgtm |
Could we revisit this? I can not see what was blowing up in Podman CI/CD system any longer. Could it just have been an ancient version of ubuntu that was blowing up? |
From what I remember (and from #969 (comment)), the issue was that Podman opens The changes in this PR modified the |
Can we fix this Patch to not do that or add a new API to check if the apparmor is loaded? |
Well, I suppose. I think the way to do that would be to only attempt to load the default profile into the kernel if we're running with uid 0, and just skip the check and loading altogether otherwise. For that to work without breaking Podman on AppArmor-enabled kernels we'd need to make the default profile
The only way to rootlessly check for the existence of a profile is to attempt a profile transition by writing to |
Well I am certainly not an expert in this, being the SELinux guy, and most of the core engineers know little of apparmor, so we need community to take the lead. If you could make this work it would be great. |
Previously, Podman would print an error if you tried to run a container
with an AppArmor profile as a non-root user, e.g.
In fact, the only thing that Podman needs root privileges for is reading
/sys/kernel/security/apparmor/profiles to see if the profile is already
loaded, which isn't strictly necessary.
This commit removes the 'IsLoaded()' check that occurs when you try to
specify an AppArmor profile as a non-root user, as well as the other
checks in pkg/apparmor/ for whether the program is running as UID 0. The
check for whether the AppArmor profile is loaded should now be deferred
to the container runtime at the point where it writes to either
/proc/self/attr/exec or /proc/self/attr/apparmor/exec, since the write
should fail if the profile is not loaded.
Closes #958.