examples/Makefile.am | 24 ++++++++++++++++++++++-- examples/apparmor/usr.lib.libvirt.virt-aa-helper | 4 ++++ 2 files changed, 26 insertions(+), 2 deletions(-)
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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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!
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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? -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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,
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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.) -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-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. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2024 Red Hat, Inc.