[libvirt] [PATCH] virt-aa-helper: handle more disk images
virt-aa-helper needs read access to the disk image to resolve symlinks and add the proper rules to the profile. Its profile whitelists a few common paths, but users can place their images anywhere. This commit helps users allowing access to their images by adding their own rules in apparmor.d/local/usr.lib.libvirt.virt-aa-helper. This commit also adds rules to allow reading files named: - *.raw as this is a rather common disk image extension - /run/libvirt/**[vd]d[a-z] as these are used by virt-sandbox --- examples/Makefile.am | 24 ++++++++++++++++++++++-- examples/apparmor/usr.lib.libvirt.virt-aa-helper | 4 ++++ 2 files changed, 26 insertions(+), 2 deletions(-) diff --git a/examples/Makefile.am b/examples/Makefile.am index ef2f79db3..8a1d6919a 100644 --- a/examples/Makefile.am +++ b/examples/Makefile.am @@ -67,6 +67,9 @@ admin_client_info_SOURCES = admin/client_info.c admin_client_close_SOURCES = admin/client_close.c admin_logging_SOURCES = admin/logging.c +INSTALL_DATA_LOCAL = +UNINSTALL_LOCAL = + if WITH_APPARMOR_PROFILES apparmordir = $(sysconfdir)/apparmor.d/ apparmor_DATA = \ @@ -85,20 +88,37 @@ templates_DATA = \ apparmor/TEMPLATE.qemu \ apparmor/TEMPLATE.lxc \ $(NULL) + +APPARMOR_LOCAL_DIR = "$(DESTDIR)$(apparmordir)/local" +install-apparmor-local: + $(MKDIR_P) "$(APPARMOR_LOCAL_DIR)" + echo "# Site-specific additions and overrides for \ + 'usr.lib.libvirt.virt-aa-helper'" \ + >$(APPARMOR_LOCAL_DIR)/usr.lib.libvirt.virt-aa-helper + +INSTALL_DATA_LOCAL += install-apparmor-local +UNINSTALL_LOCAL += uninstall-apparmor-local endif WITH_APPARMOR_PROFILES if WITH_NWFILTER NWFILTER_DIR = "$(DESTDIR)$(sysconfdir)/libvirt/nwfilter" -install-data-local: +install-nwfilter-local: $(MKDIR_P) "$(NWFILTER_DIR)" for f in $(FILTERS); do \ $(INSTALL_DATA) $$f "$(NWFILTER_DIR)"; \ done -uninstall-local:: +uninstall-nwfilter-local:: for f in $(FILTERS); do \ rm -f "$(NWFILTER_DIR)/`basename $$f`"; \ done -test -z "$(shell ls $(NWFILTER_DIR))" || rmdir $(NWFILTER_DIR) + +INSTALL_DATA_LOCAL += install-nwfilter-local +UNINSTALL_LOCAL += uninstall-nwfilter-local endif WITH_NWFILTER + +install-data-local: $(INSTALL_DATA_LOCAL) + +uninstall-local: $(UNINSTALL_LOCAL) diff --git a/examples/apparmor/usr.lib.libvirt.virt-aa-helper b/examples/apparmor/usr.lib.libvirt.virt-aa-helper index bd6181d00..f3069d369 100644 --- a/examples/apparmor/usr.lib.libvirt.virt-aa-helper +++ b/examples/apparmor/usr.lib.libvirt.virt-aa-helper @@ -3,6 +3,7 @@ profile virt-aa-helper /usr/{lib,lib64}/libvirt/virt-aa-helper { #include <abstractions/base> + #include <local/usr.lib.libvirt.virt-aa-helper> # needed for searching directories capability dac_override, @@ -50,8 +51,11 @@ profile virt-aa-helper /usr/{lib,lib64}/libvirt/virt-aa-helper { /var/lib/libvirt/images/ r, /var/lib/libvirt/images/** r, /{media,mnt,opt,srv}/** r, + # For virt-sandbox + /run/libvirt/**/[sv]d[a-z] r /**.img r, + /**.raw r, /**.qcow{,2} r, /**.qed r, /**.vmdk r, -- 2.15.1
Hi, Cédric Bosdonnat:
This commit helps users allowing access to their images by adding their own rules in apparmor.d/local/usr.lib.libvirt.virt-aa-helper. […] profile virt-aa-helper /usr/{lib,lib64}/libvirt/virt-aa-helper { #include <abstractions/base> + #include <local/usr.lib.libvirt.virt-aa-helper>
The packaging helper we use in Debian adds exactly the same line at the *end* of the profile (and actually, at the end of almost every AppArmor profile included in Debian and derivatives); I don't know why it's added at the end and not at the beginning. I suspect Jamie will know better. If there's no strong reason to add this line in the beginning of the profile, I suggest we add it at the end instead, so we avoid changing behaviour subtly once this gets merged upstream and we drop the Debian-specific line. Other than this, ACK from me on the proposed profile modifications. I am not well placed to comment on the build system changes though. Cheers!
On Tue, 2017-12-12 at 15:01 +0100, intrigeri wrote:
Hi,
Cédric Bosdonnat:
This commit helps users allowing access to their images by adding their own rules in apparmor.d/local/usr.lib.libvirt.virt-aa-helper. […] profile virt-aa-helper /usr/{lib,lib64}/libvirt/virt-aa-helper { #include <abstractions/base> + #include <local/usr.lib.libvirt.virt-aa-helper>
The packaging helper we use in Debian adds exactly the same line at the *end* of the profile (and actually, at the end of almost every AppArmor profile included in Debian and derivatives); I don't know why it's added at the end and not at the beginning. I suspect Jamie will know better.
If there's no strong reason to add this line in the beginning of the profile, I suggest we add it at the end instead, so we avoid changing behaviour subtly once this gets merged upstream and we drop the Debian-specific line.
Other than this, ACK from me on the proposed profile modifications.
I am not well placed to comment on the build system changes though.
I'm perfectly fine in having that include at the end of the profile. I'll push with that change. -- Cedric
Hi, Cedric Bosdonnat:
On Tue, 2017-12-12 at 15:01 +0100, intrigeri wrote:
Cédric Bosdonnat:
This commit helps users allowing access to their images by adding their own rules in apparmor.d/local/usr.lib.libvirt.virt-aa-helper. […] profile virt-aa-helper /usr/{lib,lib64}/libvirt/virt-aa-helper { #include <abstractions/base> + #include <local/usr.lib.libvirt.virt-aa-helper>
The packaging helper we use in Debian adds exactly the same line at the *end* of the profile (and actually, at the end of almost every AppArmor profile included in Debian and derivatives); I don't know why it's added at the end and not at the beginning. I suspect Jamie will know better.
If there's no strong reason to add this line in the beginning of the profile, I suggest we add it at the end instead, so we avoid changing behaviour subtly once this gets merged upstream and we drop the Debian-specific line.
Other than this, ACK from me on the proposed profile modifications.
I'm perfectly fine in having that include at the end of the profile. I'll push with that change.
Thanks! This will allow us to remove half of apparmor_profiles_local_include.patch we carry in Debian. Now, two follow-up questions: 1. Doing the same for usr.sbin.libvirtd? The apparmor_profiles_local_include.patch Debian patch does the same for usr.sbin.libvirtd: diff --git a/examples/apparmor/usr.sbin.libvirtd b/examples/apparmor/usr.sbin.libvirtd index fa4ebb3..5505bf6 100644 --- a/examples/apparmor/usr.sbin.libvirtd +++ b/examples/apparmor/usr.sbin.libvirtd @@ -90,4 +90,7 @@ /usr/{lib,lib64,lib/qemu,libexec}/qemu-bridge-helper rmix, } + + # Site-specific additions and overrides. See local/README for details. + #include <local/usr.sbin.libvirtd> } Cédric and others, what about upstreaming this as well? 2. Impact on packaging for distros that were already managing this local file themselves & differently … i.e. I guess mostly Debian and derivatives, that use dh_apparmor. If I got the build system changes right, local/usr.lib.libvirt.virt-aa-helper will be created at installation time so in practice it'll be shipped by distro packages. I *think* it's not a problem with dh_apparmor because the generated postinst scripts only manage that file if it does not exist yet (so it should be a no-op if dpkg has already installed it), and similarly the code for purging in postrm should work just fine if dpkg has already deleted it. But local/usr.lib.libvirt.virt-aa-helper becomes a conffile, which previously it was not managed by dpkg. I don't know how this is handled by dpkg. I suspect it might be easier to comment out: INSTALL_DATA_LOCAL += install-apparmor-local UNINSTALL_LOCAL += uninstall-apparmor-local … and keep the current behaviour, for consistency with how all other local/* override files are handled in Debian/Ubuntu. Guido, Ubuntu packagers, what do you think? Cheers, -- intrigeri
Hi, On Thu, Dec 21, 2017 at 12:10:58PM +0100, intrigeri wrote: [..snip..]
But local/usr.lib.libvirt.virt-aa-helper becomes a conffile, which previously it was not managed by dpkg. I don't know how this is handled by dpkg. I suspect it might be easier to comment out:
INSTALL_DATA_LOCAL += install-apparmor-local UNINSTALL_LOCAL += uninstall-apparmor-local
I'd do this our leave out the local/ part altogether libvirt upstream. The apparmor profiles are in examples/ and starting to add local overrides to things that live there seems weird. Cheers, -- Guido
… and keep the current behaviour, for consistency with how all other local/* override files are handled in Debian/Ubuntu.
Guido, Ubuntu packagers, what do you think?
On Thu, 2017-12-21 at 12:10 +0100, intrigeri wrote:
1. Doing the same for usr.sbin.libvirtd?
Is there any real good for the user to have local changes for the libvirtd profile?
The apparmor_profiles_local_include.patch Debian patch does the same for usr.sbin.libvirtd:
diff --git a/examples/apparmor/usr.sbin.libvirtd b/examples/apparmor/usr.sbin.libvirtd index fa4ebb3..5505bf6 100644 --- a/examples/apparmor/usr.sbin.libvirtd +++ b/examples/apparmor/usr.sbin.libvirtd @@ -90,4 +90,7 @@
/usr/{lib,lib64,lib/qemu,libexec}/qemu-bridge-helper rmix, } + + # Site-specific additions and overrides. See local/README for details. + #include <local/usr.sbin.libvirtd> }
Cédric and others, what about upstreaming this as well?
We don't have it yet in the openSUSE/SLES packages, so feel free to upstream it.
2. Impact on packaging for distros that were already managing this local file themselves & differently
… i.e. I guess mostly Debian and derivatives, that use dh_apparmor.
If I got the build system changes right, local/usr.lib.libvirt.virt-aa-helper will be created at installation time so in practice it'll be shipped by distro packages.
Hum... In any case in a spec file (and I suppose for you too) that file can be removed before creating the package. I'm not feeling good about including files in the upstream profiles that don't exist.
I *think* it's not a problem with dh_apparmor because the generated postinst scripts only manage that file if it does not exist yet (so it should be a no-op if dpkg has already installed it), and similarly the code for purging in postrm should work just fine if dpkg has already deleted it.
We mark all files in local with %config(noreplace), not sure what is best ;) -- Cedric
Hi there! Has that one landed in abyssal depths of the mailing list? -- Cedric On Mon, 2017-12-11 at 16:23 +0100, Cédric Bosdonnat wrote:
virt-aa-helper needs read access to the disk image to resolve symlinks and add the proper rules to the profile. Its profile whitelists a few common paths, but users can place their images anywhere.
This commit helps users allowing access to their images by adding their own rules in apparmor.d/local/usr.lib.libvirt.virt-aa-helper.
This commit also adds rules to allow reading files named: - *.raw as this is a rather common disk image extension - /run/libvirt/**[vd]d[a-z] as these are used by virt-sandbox --- examples/Makefile.am | 24 ++++++++++++++++++++++-- examples/apparmor/usr.lib.libvirt.virt-aa-helper | 4 ++++ 2 files changed, 26 insertions(+), 2 deletions(-)
diff --git a/examples/Makefile.am b/examples/Makefile.am index ef2f79db3..8a1d6919a 100644 --- a/examples/Makefile.am +++ b/examples/Makefile.am @@ -67,6 +67,9 @@ admin_client_info_SOURCES = admin/client_info.c admin_client_close_SOURCES = admin/client_close.c admin_logging_SOURCES = admin/logging.c
+INSTALL_DATA_LOCAL = +UNINSTALL_LOCAL = + if WITH_APPARMOR_PROFILES apparmordir = $(sysconfdir)/apparmor.d/ apparmor_DATA = \ @@ -85,20 +88,37 @@ templates_DATA = \ apparmor/TEMPLATE.qemu \ apparmor/TEMPLATE.lxc \ $(NULL) + +APPARMOR_LOCAL_DIR = "$(DESTDIR)$(apparmordir)/local" +install-apparmor-local: + $(MKDIR_P) "$(APPARMOR_LOCAL_DIR)" + echo "# Site-specific additions and overrides for \ + 'usr.lib.libvirt.virt-aa-helper'" \ + >$(APPARMOR_LOCAL_DIR)/usr.lib.libvirt.virt-aa-helper + +INSTALL_DATA_LOCAL += install-apparmor-local +UNINSTALL_LOCAL += uninstall-apparmor-local endif WITH_APPARMOR_PROFILES
if WITH_NWFILTER NWFILTER_DIR = "$(DESTDIR)$(sysconfdir)/libvirt/nwfilter"
-install-data-local: +install-nwfilter-local: $(MKDIR_P) "$(NWFILTER_DIR)" for f in $(FILTERS); do \ $(INSTALL_DATA) $$f "$(NWFILTER_DIR)"; \ done
-uninstall-local:: +uninstall-nwfilter-local:: for f in $(FILTERS); do \ rm -f "$(NWFILTER_DIR)/`basename $$f`"; \ done -test -z "$(shell ls $(NWFILTER_DIR))" || rmdir $(NWFILTER_DIR) + +INSTALL_DATA_LOCAL += install-nwfilter-local +UNINSTALL_LOCAL += uninstall-nwfilter-local endif WITH_NWFILTER + +install-data-local: $(INSTALL_DATA_LOCAL) + +uninstall-local: $(UNINSTALL_LOCAL) diff --git a/examples/apparmor/usr.lib.libvirt.virt-aa-helper b/examples/apparmor/usr.lib.libvirt.virt-aa-helper index bd6181d00..f3069d369 100644 --- a/examples/apparmor/usr.lib.libvirt.virt-aa-helper +++ b/examples/apparmor/usr.lib.libvirt.virt-aa-helper @@ -3,6 +3,7 @@
profile virt-aa-helper /usr/{lib,lib64}/libvirt/virt-aa-helper { #include <abstractions/base> + #include <local/usr.lib.libvirt.virt-aa-helper>
# needed for searching directories capability dac_override, @@ -50,8 +51,11 @@ profile virt-aa-helper /usr/{lib,lib64}/libvirt/virt-aa-helper { /var/lib/libvirt/images/ r, /var/lib/libvirt/images/** r, /{media,mnt,opt,srv}/** r, + # For virt-sandbox + /run/libvirt/**/[sv]d[a-z] r
/**.img r, + /**.raw r, /**.qcow{,2} r, /**.qed r, /**.vmdk r,
Hi, Cedric Bosdonnat:
Has that one landed in abyssal depths of the mailing list?
Well, no, it's waiting for your comments about my feedback: https://www.redhat.com/archives/libvir-list/2017-December/msg00389.html Thanks for pinging! (Sorry I did not put you in explicit copy, I assumed you would monitor replies on the list.)
On Wed, 2017-12-20 at 10:17 +0100, intrigeri wrote:
Hi,
Cedric Bosdonnat:
Has that one landed in abyssal depths of the mailing list?
Well, no, it's waiting for your comments about my feedback: https://www.redhat.com/archives/libvir-list/2017-December/msg00389.html
Thanks for pinging!
(Sorry I did not put you in explicit copy, I assumed you would monitor replies on the list.)
Oops, I overlooked that one.
On Mon, 2017-12-11 at 16:23 +0100, Cédric Bosdonnat wrote: ...
diff --git a/examples/apparmor/usr.lib.libvirt.virt-aa-helper b/examples/apparmor/usr.lib.libvirt.virt-aa-helper index bd6181d00..f3069d369 100644 --- a/examples/apparmor/usr.lib.libvirt.virt-aa-helper +++ b/examples/apparmor/usr.lib.libvirt.virt-aa-helper @@ -3,6 +3,7 @@
profile virt-aa-helper /usr/{lib,lib64}/libvirt/virt-aa-helper { #include <abstractions/base> + #include <local/usr.lib.libvirt.virt-aa-helper>
# needed for searching directories capability dac_override, @@ -50,8 +51,11 @@ profile virt-aa-helper /usr/{lib,lib64}/libvirt/virt-aa-helper { /var/lib/libvirt/images/ r, /var/lib/libvirt/images/** r, /{media,mnt,opt,srv}/** r, + # For virt-sandbox + /run/libvirt/**/[sv]d[a-z] r
/**.img r, + /**.raw r, /**.qcow{,2} r, /**.qed r, /**.vmdk r,
These profile changes LGTM. +1 to apply them. Like intrigeri, I'll let someone else ACK the build system changes. -- Jamie Strandboge | http://www.canonical.com
participants (5)
-
Cedric Bosdonnat -
Cédric Bosdonnat
-
Guido Günther -
intrigeri -
Jamie Strandboge