[libvirt] [PATCH v3 3/3] security_dac: Favour ACLs over chown()

[Daniel P. Berrange]

On Mon, Mar 11, 2013 at 05:13:29PM +0100, Michal Privoznik wrote:
> On filesystems supporting ACLs we don't need to do a chown but we
> can just set ACLs to gain access for qemu. However, since we are
> setting these on too low level, where we don't know if disk is
> just a read only or read write, we set read write access
> unconditionally.
> 
> >From implementation POV, a reference counter is introduced, so ACL is
> restored only on the last restore attempt in order to not cut off other
> domains. And since a file may had an ACL for a user already set, we need
> to keep this as well. Both these, the reference counter and original ACL
> are stored as extended attributes named trusted.refCount and
> trusted.oldACL respectively.
> ---
> 
> diff to v2:
> -basically squashed functionality of 2/4 and 4/4 from previous
> round
> 
>  src/security/security_dac.c | 209 ++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 182 insertions(+), 27 deletions(-)
> 
> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
> index 0b274b7..46cc686 100644
> --- a/src/security/security_dac.c
> +++ b/src/security/security_dac.c
> @@ -25,6 +25,7 @@
>  
>  #include "security_dac.h"
>  #include "virerror.h"
> +#include "virfile.h"
>  #include "virutil.h"
>  #include "viralloc.h"
>  #include "virlog.h"
> @@ -34,6 +35,8 @@
>  
>  #define VIR_FROM_THIS VIR_FROM_SECURITY
>  #define SECURITY_DAC_NAME "dac"
> +#define SECURITY_DAC_XATTR_OLD_ACL "trusted.oldACL"
> +#define SECURITY_DAC_XATTR_REFCOUNT "trusted.refCount"

I think we want 'libvirt.dac' as a prefix for any xattrs we set
from this DAC driver.

> @@ -265,11 +406,8 @@ static const char * virSecurityDACGetDOI(virSecurityManagerPtr mgr ATTRIBUTE_UNU
>  }
>  
>  static int
> -virSecurityDACSetOwnership(const char *path, uid_t uid, gid_t gid)
> +virSecurityDACChown(const char *path, uid_t uid, gid_t gid)
>  {
> -    VIR_INFO("Setting DAC user and group on '%s' to '%ld:%ld'",
> -             path, (long) uid, (long) gid);
> -
>      if (chown(path, uid, gid) < 0) {
>          struct stat sb;
>          int chown_errno = errno;

In the traditional chown() codepath, you're never preserving the original
uid/gid values in an xattr. It not unknown for admins to mount filesystems
with '-o noacl', which blocks use of ACLs, but still allows for xattrs to
be used by apps. So we can preserve uid/gid in this case

> +
> +    /* Oops, something went wrong. If ACL or XATTR are not
> +     * supported fall back to chown then. */
>  
> -    /* XXX record previous ownership */
> -    rc = virSecurityDACSetOwnership(newpath, 0, 0);
> +    rc = virSecurityDACChown(path, 0, 0);

Dropping this comment is bogus since you've not fixed the problem
it referred to.

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|