[libvirt] [PATCH v2 4/4] security_dac: Favour ACLs over chown()

[Daniel P. Berrange]

On Wed, Mar 06, 2013 at 03:05:56PM +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.
> ---
>  src/security/security_dac.c | 32 +++++++++++++++++++++++++++-----
>  1 file changed, 27 insertions(+), 5 deletions(-)
> 
> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
> index 76a1dc6..8805a5b 100644
> --- a/src/security/security_dac.c
> +++ b/src/security/security_dac.c
> @@ -329,12 +329,12 @@ virSecurityDACGetRememberedLabel(const char *path,
>              goto cleanup;
>          ret = refCount;
>      } else {
> -        if (virFileGetAttr(path, SECURITY_DAC_XATTR_OLD_OWNER, &oldOwner) < 0 ||
> -            !oldOwner)
> -            return ret;
> +        if (!virFileGetAttr(path, SECURITY_DAC_XATTR_OLD_OWNER, &oldOwner) &&
> +            oldOwner) {
>  
> -        if (parseIds(oldOwner, user, group) < 0)
> -            goto cleanup;
> +            if (parseIds(oldOwner, user, group) < 0)
> +                goto cleanup;
> +        }
>  
>          virFileRemoveAttr(path, SECURITY_DAC_XATTR_REFCOUNT);
>          virFileRemoveAttr(path, SECURITY_DAC_XATTR_OLD_OWNER);
> @@ -384,6 +384,9 @@ virSecurityDACSetOwnership(const char *path,
>                             gid_t gid,
>                             bool remember)
>  {
> +    virErrorPtr err;
> +    int rv;
> +
>      VIR_INFO("Setting DAC user and group on '%s' to '%ld:%ld'",
>               path, (long) uid, (long) gid);
>  
> @@ -391,6 +394,25 @@ virSecurityDACSetOwnership(const char *path,
>          virSecurityDACRememberLabel(path) < 0)
>          return -1;
>  
> +    if (remember) {
> +        if ((rv = virFileSetACL(path, uid, S_IRUSR | S_IWUSR)) == 0) {
> +            /* No chown is necessary, so remove oldOwner xattr. */
> +            virFileRemoveAttr(path, SECURITY_DAC_XATTR_OLD_OWNER);

Ewww, just re-arrange this code, so that you don't set the xattr
in the first place when you use ACLs.

> +        }
> +    } else {
> +        rv = virFileRemoveACL(path, uid);

And this should just be done directly in the restore method


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 :|