9:23 a.m.
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
8:01 a.m.
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!
3:42 a.m.
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
5:10 a.m.
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
5:28 a.m.
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?
6:45 a.m.
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
6:16 a.m.
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,
3:17 a.m.
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.)
3:41 a.m.
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.
12:33 p.m.
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
2529
days inactive
2539
days old
9 comments
5 participants
participants (5)
-
Cedric Bosdonnat -
Cédric Bosdonnat
-
Guido Günther -
intrigeri -
Jamie Strandboge