-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Support default profile for apparmor #6992
Conversation
@vrothberg @saschagrunert PTAL. I don't have access right now to systems that run with apparmor enabled. but I think this will fix some of the issues we have with it right now. I can hand this PR off to an Intern to deal with it, if neither of you want to take it and run with it. |
The goal is to make sure we fix #6933 |
LGTM, but someone who actually know the Apparmor code should review and merge |
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, now I'd love to see an e2e test for that :) I think the Ubuntu machines have it enabled by default
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rhatdan, 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 |
Still flying blind, but added a new run_apparmor_test.go test, no way to test this locally, at this point, so will see how it does. |
f5c7e32
to
8abadf2
Compare
@saschagrunert Could you try this out on an Apparmor enabled machine. The tests are failing only on the default test for Ubuntu. Testing locally on my Ubuntu VM, they pass. All of the other tests, pass, I have no idea why the default test is failing. Basically it creates a container and then makes sure the default apparmor policy is defined in the inspect code. |
Works locally on my side. This line looks suspicious:
We can also add something like
to debug the current apparmor status. |
Currently you can not apply an ApparmorProfile if you specify --privileged. This patch will allow both to be specified simultaniosly. By default Apparmor should be disabled if the user specifies --privileged, but if the user specifies --security apparmor:PROFILE, with --privileged, we should do both. Added e2e run_apparmor_test.go Signed-off-by: Daniel J Walsh <[email protected]>
Thanks @saschagrunert Nice catch. Does containers-common on ubuntu automatically install an /etc/containers/containers.conf? |
Yes, the file looks like this on Ubuntu 20.04:
|
Ok, I don't believe we are shipping this file in Fedora, only /usr/share/containers/containers.conf. |
@saschagrunert Looks like that was the problem. Thanks for finding it. |
LGTM Let's get this merged so we can backport to the v2.0 branch for 2.0.3 - I'd like to have this fix in for the Ubuntu and Debian folks. |
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
Currently you can not apply an ApparmorProfile if you specify
--privileged. This patch will allow both to be specified
simultaniosly.
By default Apparmor should be disabled if the user
specifies --privileged, but if the user specifies --security apparmor:PROFILE,
with --privileged, we should do both.
Signed-off-by: Daniel J Walsh [email protected]