[PATCH 1/2] apparmor: allow adding permanent per guest rules
The design of apparmor in libvirt always had a way to define custom per-guest rules as described in docs/drvqemu.html and [1]. A fix meant to clean the profiles after guest shutdown was a bit overzealous and accidentially removed this important admin feature as well. Therefore reduce the --delete option of virt-aa-helper to only delete the .files that would be re-generated in any case. Users/Admins are always free to clean the profiles themselve if they prefer a clean directory - they will be regenerated as needed. But libvirt should never remove the base profile meant to allow per-guest overrides and thereby break a documented feature. [1]: https://gitlab.com/apparmor/apparmor/-/wikis/Libvirt#advanced-usage Fixes: eba2225b "apparmor: delete profile on VM shutdown" Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- src/security/virt-aa-helper.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index dadb9d1614..4b66422b8f 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -99,7 +99,7 @@ vah_usage(void) " Modes:\n" " -a | --add load profile\n" " -c | --create create profile from template\n" - " -D | --delete unload and delete profile\n" + " -D | --delete unload profile and delete generated rules\n" " -r | --replace reload profile\n" " -R | --remove unload profile\n" " Options:\n" @@ -1491,7 +1491,6 @@ main(int argc, char **argv) rc = parserRemove(ctl->uuid); if (ctl->cmd == 'D') { unlink(include_file); - unlink(profile); } } else if (ctl->cmd == 'c' || ctl->cmd == 'r') { char *included_files = NULL; -- 2.27.0
With qemu 5.0 and libvirt 6.6 there are new apparmor denials: apparmor="DENIED" operation="umount" profile="libvirtd" name="/run/libvirt/qemu/1-kvmguest-groovy-norm.dev/" comm="rpc-worker" These are related to new issues around devmapper handling [1] and the error path triggered by these issues now causes this new denial. There are already related rules for mounting and it seems right to allow also the related umount. [1]: https://www.redhat.com/archives/libvir-list/2020-August/msg00236.html Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- src/security/apparmor/usr.sbin.libvirtd.in | 1 + 1 file changed, 1 insertion(+) diff --git a/src/security/apparmor/usr.sbin.libvirtd.in b/src/security/apparmor/usr.sbin.libvirtd.in index 1e137039e9..7c48b36e3d 100644 --- a/src/security/apparmor/usr.sbin.libvirtd.in +++ b/src/security/apparmor/usr.sbin.libvirtd.in @@ -31,6 +31,7 @@ profile libvirtd @sbindir@/libvirtd flags=(attach_disconnected) { mount options=(rw,rslave) -> /, mount options=(rw, nosuid) -> /{var/,}run/libvirt/qemu/*.dev/, + umount /{var/,}run/libvirt/qemu/*.dev/, # libvirt provides any mounts under /dev to qemu namespaces mount options=(rw, move) /dev/ -> /{,var/}run/libvirt/qemu/*.dev/, -- 2.27.0
On Fri, Aug 07, 2020 at 12:21:20PM +0200, Christian Ehrhardt wrote:
With qemu 5.0 and libvirt 6.6 there are new apparmor denials: apparmor="DENIED" operation="umount" profile="libvirtd" name="/run/libvirt/qemu/1-kvmguest-groovy-norm.dev/" comm="rpc-worker"
These are related to new issues around devmapper handling [1] and the error path triggered by these issues now causes this new denial.
There are already related rules for mounting and it seems right to allow also the related umount.
[1]: https://www.redhat.com/archives/libvir-list/2020-August/msg00236.html
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- src/security/apparmor/usr.sbin.libvirtd.in | 1 + 1 file changed, 1 insertion(+)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Fri, Aug 7, 2020 at 6:14 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
On Fri, Aug 07, 2020 at 12:21:20PM +0200, Christian Ehrhardt wrote:
With qemu 5.0 and libvirt 6.6 there are new apparmor denials: apparmor="DENIED" operation="umount" profile="libvirtd" name="/run/libvirt/qemu/1-kvmguest-groovy-norm.dev/" comm="rpc-worker"
These are related to new issues around devmapper handling [1] and the error path triggered by these issues now causes this new denial.
There are already related rules for mounting and it seems right to allow also the related umount.
[1]: https://www.redhat.com/archives/libvir-list/2020-August/msg00236.html
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- src/security/apparmor/usr.sbin.libvirtd.in | 1 + 1 file changed, 1 insertion(+)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Thanks for the review - there was no negative feedback so far and in tests this worked fine. I'm committing the changes to not be postponed to close to the next release.
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
-- Christian Ehrhardt Staff Engineer, Ubuntu Server Canonical Ltd
On Fri, Aug 07, 2020 at 12:21:19PM +0200, Christian Ehrhardt wrote:
The design of apparmor in libvirt always had a way to define custom per-guest rules as described in docs/drvqemu.html and [1].
A fix meant to clean the profiles after guest shutdown was a bit overzealous and accidentially removed this important admin feature as well.
Therefore reduce the --delete option of virt-aa-helper to only delete the .files that would be re-generated in any case.
Users/Admins are always free to clean the profiles themselve if they prefer a clean directory - they will be regenerated as needed. But libvirt should never remove the base profile meant to allow per-guest overrides and thereby break a documented feature.
[1]: https://gitlab.com/apparmor/apparmor/-/wikis/Libvirt#advanced-usage
Fixes: eba2225b "apparmor: delete profile on VM shutdown"
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- src/security/virt-aa-helper.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Fri, Aug 7, 2020 at 6:14 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
On Fri, Aug 07, 2020 at 12:21:19PM +0200, Christian Ehrhardt wrote:
The design of apparmor in libvirt always had a way to define custom per-guest rules as described in docs/drvqemu.html and [1].
A fix meant to clean the profiles after guest shutdown was a bit overzealous and accidentially removed this important admin feature as well.
Therefore reduce the --delete option of virt-aa-helper to only delete the .files that would be re-generated in any case.
Users/Admins are always free to clean the profiles themselve if they prefer a clean directory - they will be regenerated as needed. But libvirt should never remove the base profile meant to allow per-guest overrides and thereby break a documented feature.
[1]: https://gitlab.com/apparmor/apparmor/-/wikis/Libvirt#advanced-usage
Fixes: eba2225b "apparmor: delete profile on VM shutdown"
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com> --- src/security/virt-aa-helper.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
(as with the other recent apparmor patch patch) Thanks for the review - there was no negative feedback so far and in tests this worked fine. I'm committing the changes to not be postponed to close to the next release.
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
-- Christian Ehrhardt Staff Engineer, Ubuntu Server Canonical Ltd
participants (2)
-
Christian Ehrhardt -
Daniel P. Berrangé