[libvirt] [PATCH 2/4] configure: Make ACL mandatory when building the QEMU driver

Andrea Bolognani posted 4 patches 157 weeks ago

[libvirt] [PATCH 2/4] configure: Make ACL mandatory when building the QEMU driver

Posted by Andrea Bolognani 157 weeks ago
When we're building a private /dev for the isolated QEMU
process, we want to be able to replicate the contents of
the original /dev as closely as possible, including ACLs.

To ensure that's always possible, make ACL support mandatory
when the QEMU driver is enabled.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1421036
---
 m4/virt-acl.m4 | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/m4/virt-acl.m4 b/m4/virt-acl.m4
index f7d1c6d..7a8b8e5 100644
--- a/m4/virt-acl.m4
+++ b/m4/virt-acl.m4
@@ -21,6 +21,10 @@ AC_DEFUN([LIBVIRT_CHECK_ACL], [
 
   AC_CHECK_HEADERS([sys/acl.h])
 
+  if test "x$ac_cv_header_sys_acl_h:x$with_qemu" = "xno:xyes"; then
+    AC_MSG_ERROR([Unable to find <sys/acl.h>, required by qemu driver])
+  fi
+
   ACL_CFLAGS=""
   ACL_LIBS=""
   if test "x$ac_cv_header_sys_acl_h:x$with_linux" = "xyes:xyes"; then
-- 
2.7.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 2/4] configure: Make ACL mandatory when building the QEMU driver

Posted by Daniel P. Berrange 157 weeks ago
On Tue, Feb 14, 2017 at 04:33:09PM +0100, Andrea Bolognani wrote:
> When we're building a private /dev for the isolated QEMU
> process, we want to be able to replicate the contents of
> the original /dev as closely as possible, including ACLs.
> 
> To ensure that's always possible, make ACL support mandatory
> when the QEMU driver is enabled.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1421036
> ---
>  m4/virt-acl.m4 | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/m4/virt-acl.m4 b/m4/virt-acl.m4
> index f7d1c6d..7a8b8e5 100644
> --- a/m4/virt-acl.m4
> +++ b/m4/virt-acl.m4
> @@ -21,6 +21,10 @@ AC_DEFUN([LIBVIRT_CHECK_ACL], [
>  
>    AC_CHECK_HEADERS([sys/acl.h])
>  
> +  if test "x$ac_cv_header_sys_acl_h:x$with_qemu" = "xno:xyes"; then
> +    AC_MSG_ERROR([Unable to find <sys/acl.h>, required by qemu driver])
> +  fi

I understand the desire to simplify the code by assuming the libacl
APIs always exist, but we shouldn't do it this way. Instead we should
follow our normal practice of creating a src/util/viracl.{c,h} file
which wrap the APIs and providing no-op stubs which just raise an
error when the library is missing at build time.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 2/4] configure: Make ACL mandatory when building the QEMU driver

Posted by Michal Privoznik 157 weeks ago
On 02/14/2017 04:40 PM, Daniel P. Berrange wrote:
> On Tue, Feb 14, 2017 at 04:33:09PM +0100, Andrea Bolognani wrote:
>> When we're building a private /dev for the isolated QEMU
>> process, we want to be able to replicate the contents of
>> the original /dev as closely as possible, including ACLs.
>>
>> To ensure that's always possible, make ACL support mandatory
>> when the QEMU driver is enabled.
>>
>> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1421036
>> ---
>>  m4/virt-acl.m4 | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/m4/virt-acl.m4 b/m4/virt-acl.m4
>> index f7d1c6d..7a8b8e5 100644
>> --- a/m4/virt-acl.m4
>> +++ b/m4/virt-acl.m4
>> @@ -21,6 +21,10 @@ AC_DEFUN([LIBVIRT_CHECK_ACL], [
>>  
>>    AC_CHECK_HEADERS([sys/acl.h])
>>  
>> +  if test "x$ac_cv_header_sys_acl_h:x$with_qemu" = "xno:xyes"; then
>> +    AC_MSG_ERROR([Unable to find <sys/acl.h>, required by qemu driver])
>> +  fi
> 
> I understand the desire to simplify the code by assuming the libacl
> APIs always exist, but we shouldn't do it this way. Instead we should
> follow our normal practice of creating a src/util/viracl.{c,h} file
> which wrap the APIs and providing no-op stubs which just raise an
> error when the library is missing at build time.
> 

Unfortunately, it's more complicated than that. What if your distro has
getfacl/setfacl but missing the header files (libacl-devel)? In that
case libvirt is built without ACL support. Now imagine you have a
device, say /dev/kvm with 0777 and ACL entry that denies specific
group/user. If we build without ACL support, and use namespaces we will
in fact allow that group/user to start a domain because we just create a
device with 0777 perms and do not copy ACL entries.

Or vice versa - I explicitly allow user under which I'm running in ACLs
to /dev/kvm while it's perms are 0660. Lacking libacl headers would
render me unable to use namespaces.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 2/4] configure: Make ACL mandatory when building the QEMU driver

Posted by Daniel P. Berrange 157 weeks ago
On Tue, Feb 14, 2017 at 04:55:05PM +0100, Michal Privoznik wrote:
> On 02/14/2017 04:40 PM, Daniel P. Berrange wrote:
> > On Tue, Feb 14, 2017 at 04:33:09PM +0100, Andrea Bolognani wrote:
> >> When we're building a private /dev for the isolated QEMU
> >> process, we want to be able to replicate the contents of
> >> the original /dev as closely as possible, including ACLs.
> >>
> >> To ensure that's always possible, make ACL support mandatory
> >> when the QEMU driver is enabled.
> >>
> >> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1421036
> >> ---
> >>  m4/virt-acl.m4 | 4 ++++
> >>  1 file changed, 4 insertions(+)
> >>
> >> diff --git a/m4/virt-acl.m4 b/m4/virt-acl.m4
> >> index f7d1c6d..7a8b8e5 100644
> >> --- a/m4/virt-acl.m4
> >> +++ b/m4/virt-acl.m4
> >> @@ -21,6 +21,10 @@ AC_DEFUN([LIBVIRT_CHECK_ACL], [
> >>  
> >>    AC_CHECK_HEADERS([sys/acl.h])
> >>  
> >> +  if test "x$ac_cv_header_sys_acl_h:x$with_qemu" = "xno:xyes"; then
> >> +    AC_MSG_ERROR([Unable to find <sys/acl.h>, required by qemu driver])
> >> +  fi
> > 
> > I understand the desire to simplify the code by assuming the libacl
> > APIs always exist, but we shouldn't do it this way. Instead we should
> > follow our normal practice of creating a src/util/viracl.{c,h} file
> > which wrap the APIs and providing no-op stubs which just raise an
> > error when the library is missing at build time.
> > 
> 
> Unfortunately, it's more complicated than that. What if your distro has
> getfacl/setfacl but missing the header files (libacl-devel)? In that
> case libvirt is built without ACL support. Now imagine you have a
> device, say /dev/kvm with 0777 and ACL entry that denies specific
> group/user. If we build without ACL support, and use namespaces we will
> in fact allow that group/user to start a domain because we just create a
> device with 0777 perms and do not copy ACL entries.
> 
> Or vice versa - I explicitly allow user under which I'm running in ACLs
> to /dev/kvm while it's perms are 0660. Lacking libacl headers would
> render me unable to use namespaces.

Your platform has libacl available so it is not difficult to fix that
by building with libacl support. We print out the configure summary
precisely so users can see if there's any libraries they forgot to
install which might be useful.

Mandating libacl will prevent use of the QEMU driver on platforms lacking
the libacl library.  I see various bug reports indicating portability
problems for libacl wrt other platforms, in particular OS-X. So IMHO it
is not acceptable to make it a mandatory requirement, when there's no
good reason for that aside beyond helping people who forget to install
-devel library packages

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 2/4] configure: Make ACL mandatory when building the QEMU driver

Posted by Andrea Bolognani 157 weeks ago
On Tue, 2017-02-14 at 16:00 +0000, Daniel P. Berrange wrote:
> Your platform has libacl available so it is not difficult to fix that
> by building with libacl support. We print out the configure summary
> precisely so users can see if there's any libraries they forgot to
> install which might be useful.
> 
> Mandating libacl will prevent use of the QEMU driver on platforms lacking
> the libacl library.  I see various bug reports indicating portability
> problems for libacl wrt other platforms, in particular OS-X. So IMHO it
> is not acceptable to make it a mandatory requirement, when there's no
> good reason for that aside beyond helping people who forget to install
> -devel library packages

FreeBSD and other have <sys/acl.h>, it's just not part of a
separate package but of libc itself. Not sure about macOS,
but it being a BSD derivative I expect it wouldn't be too
different.

On the other hand, we really only care about having the ACL
APIs when we are isolating QEMU, which only happens of Linux
due to the namespaces requirement... So maybe we could have
it as a strict requirement on Linux only, and as an optional
dependency on other platforms?

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 2/4] configure: Make ACL mandatory when building the QEMU driver

Posted by Daniel P. Berrange 157 weeks ago
On Tue, Feb 14, 2017 at 05:14:33PM +0100, Andrea Bolognani wrote:
> On Tue, 2017-02-14 at 16:00 +0000, Daniel P. Berrange wrote:
> > Your platform has libacl available so it is not difficult to fix that
> > by building with libacl support. We print out the configure summary
> > precisely so users can see if there's any libraries they forgot to
> > install which might be useful.
> > 
> > Mandating libacl will prevent use of the QEMU driver on platforms lacking
> > the libacl library.  I see various bug reports indicating portability
> > problems for libacl wrt other platforms, in particular OS-X. So IMHO it
> > is not acceptable to make it a mandatory requirement, when there's no
> > good reason for that aside beyond helping people who forget to install
> > -devel library packages
> 
> FreeBSD and other have <sys/acl.h>, it's just not part of a
> separate package but of libc itself. Not sure about macOS,
> but it being a BSD derivative I expect it wouldn't be too
> different.
> 
> On the other hand, we really only care about having the ACL
> APIs when we are isolating QEMU, which only happens of Linux
> due to the namespaces requirement... So maybe we could have
> it as a strict requirement on Linux only, and as an optional
> dependency on other platforms?

IMHO it'd be better to just disable the namespace code at build
time if we don't have libacl rather than adding mandatory build
deps.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 2/4] configure: Make ACL mandatory when building the QEMU driver

Posted by Andrea Bolognani 157 weeks ago
On Tue, 2017-02-14 at 16:20 +0000, Daniel P. Berrange wrote:
> > On the other hand, we really only care about having the ACL
> > APIs when we are isolating QEMU, which only happens of Linux
> > due to the namespaces requirement... So maybe we could have
> > it as a strict requirement on Linux only, and as an optional
> > dependency on other platforms?
> 
> IMHO it'd be better to just disable the namespace code at build
> time if we don't have libacl rather than adding mandatory build
> deps.

I'm afraid that might lead to people forgetting to install
libacl-devel[1] on Linux and ending up with less security
than expected / desired as a result.

Moreover, we're talking about a package which is literally
35k in size: I would be way more inclined to pay the price
in increased code complexity if we were not dealing with
what will basically end up as a rounding error on any
reasonable hypervisor host.

Not to mention systemd depends on it, so it will be part of
the core package set on most modern Linux distributions.


[1] I know I did while trying to figure this bug out ;)
-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 2/4] configure: Make ACL mandatory when building the QEMU driver

Posted by Daniel P. Berrange 157 weeks ago
On Tue, Feb 14, 2017 at 05:47:27PM +0100, Andrea Bolognani wrote:
> On Tue, 2017-02-14 at 16:20 +0000, Daniel P. Berrange wrote:
> > > On the other hand, we really only care about having the ACL
> > > APIs when we are isolating QEMU, which only happens of Linux
> > > due to the namespaces requirement... So maybe we could have
> > > it as a strict requirement on Linux only, and as an optional
> > > dependency on other platforms?
> > 
> > IMHO it'd be better to just disable the namespace code at build
> > time if we don't have libacl rather than adding mandatory build
> > deps.
> 
> I'm afraid that might lead to people forgetting to install
> libacl-devel[1] on Linux and ending up with less security
> than expected / desired as a result.

You can make the same argument about many other libraries we have
optional dependancies against, libcapng, libselinux, apparmour,
etc.

Our general policy is for libraries to be optional and I don't
see a reason for this to be a different case

> [1] I know I did while trying to figure this bug out ;)

If we disabled namespace support when libacl is missing at
build time you would have noticed quite quickly that you
weren't using namespaces.


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 2/4] configure: Make ACL mandatory when building the QEMU driver

Posted by Andrea Bolognani 157 weeks ago
On Tue, 2017-02-14 at 16:59 +0000, Daniel P. Berrange wrote:
> > [1] I know I did while trying to figure this bug out ;)
> 
> If we disabled namespace support when libacl is missing at
> build time you would have noticed quite quickly that you
> weren't using namespaces.

Not sure how quickly you'd notice that, it's not like we
would print a big fat warning either at configure time or
when starting a guest, so unless you were looking for it
specifically it would be entirely silent: the guest would
start anyway, it just wouldn't be isolated.

But okay, let's leave making sure all optional dependencies
are in place to users, and most importantly distributions.
As you say, it's consistent with how we deal with other
external libraries.

Michal, can you please take care of cooking up a patch to
disable namespaces when ACL support is not available?

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list