This commit adds permissions inheritance to volume from main pool when
it is not explicitly added by command or XML definition. It permissions
are defined into XML, they should be respected.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=677242

Signed-off-by: Julio Faracco <jcfaracco@gmail.com>
---
 src/storage/storage_driver.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
index 4a13e90481..5961d35f26 100644
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -1850,6 +1850,16 @@ storageVolCreateXML(virStoragePoolPtr pool,
         goto cleanup;
     }
 
+    /* Inherit perms and mode from pool when they are not defined. */
+    if (voldef->target.perms->uid == (uid_t)-1)
+        voldef->target.perms->uid = def->target.perms.uid;
+
+    if (voldef->target.perms->gid == (gid_t)-1)
+        voldef->target.perms->gid = def->target.perms.gid;
+
+    if (voldef->target.perms->mode == (mode_t)-1)
+        voldef->target.perms->mode = def->target.perms.mode;
+
     if (virStorageVolCreateXMLEnsureACL(pool->conn, def, voldef) < 0)
         goto cleanup;
 
-- 
2.19.1

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

On 1/18/19 11:00 AM, Julio Faracco wrote:
> This commit adds permissions inheritance to volume from main pool when
> it is not explicitly added by command or XML definition. It permissions
> are defined into XML, they should be respected.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=677242
> 
> Signed-off-by: Julio Faracco <jcfaracco@gmail.com>
> ---
>  src/storage/storage_driver.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 

Although I see the point of the referenced bz - the bz and patch goes
against the documented behavior, see:

https://libvirt.org/formatstorage.html

...
Storage volume XML
...
Target elements
...
permissions
...

"The mode element contains the octal permission set. The mode defaults
to 0600 when not provided. The owner element contains the numeric user
ID. The group element contains the numeric group ID. If owner or group
aren't specified when creating a supported volume, the values are
inherited from the parent directory."

Check out commit 7c2d65dd and fafcc818f for doc and code changes which
are the source of today's logic. If you follow those changes, you'll see
that creation of pool and volume has always had some sort of default;
however, determining when that default was provided or when perhaps user
provided that same value is impossible. So, the usage of -1 for fields
was done in order to help make that determination (for output purposes).

If you did change to use this default, then what would that do to the
existing code using VIR_STORAGE_DEFAULT_VOL_PERM_MODE (O600)? Or how
would code distinguish between something a consumer set and what is/was
set "by default".

BTW: If you took using a storage pool permissions to be the permissions
for the volume, then you'd need to consider that the default storage
pool permissions is handled via VIR_STORAGE_DEFAULT_POOL_PERM_MODE
(O711) which I hope you can agree/understand why you may not want that
as your storage volume value (e.g. executable).

In the long run, defaults are defaults and if someone wants something
other than the default, then they have the capability via input XML to
change those defaults. One thing that could be done is adding mode, gid,
and uid values for input for virsh vol-create-as which creates XML like
would be input during vol-create.

Another possibility for a resolution - maybe add something to a pool's
config or attributes that tells volume creation it's preferable to match
pool permissions rather than take the default when the volume XML
doesn't provide it's own permissions. Maybe that's how you solve that
bug. Maybe there's other opinions out there...

John


> diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
> index 4a13e90481..5961d35f26 100644
> --- a/src/storage/storage_driver.c
> +++ b/src/storage/storage_driver.c
> @@ -1850,6 +1850,16 @@ storageVolCreateXML(virStoragePoolPtr pool,
>          goto cleanup;
>      }
>  
> +    /* Inherit perms and mode from pool when they are not defined. */
> +    if (voldef->target.perms->uid == (uid_t)-1)
> +        voldef->target.perms->uid = def->target.perms.uid;
> +
> +    if (voldef->target.perms->gid == (gid_t)-1)
> +        voldef->target.perms->gid = def->target.perms.gid;
> +
> +    if (voldef->target.perms->mode == (mode_t)-1)
> +        voldef->target.perms->mode = def->target.perms.mode;
> +
>      if (virStorageVolCreateXMLEnsureACL(pool->conn, def, voldef) < 0)
>          goto cleanup;
>  
> 

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

On 1/25/19 4:52 PM, John Ferlan wrote:
> 
> 
> On 1/18/19 11:00 AM, Julio Faracco wrote:
>> This commit adds permissions inheritance to volume from main pool when
>> it is not explicitly added by command or XML definition. It permissions
>> are defined into XML, they should be respected.
>>
>> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=677242
>>
>> Signed-off-by: Julio Faracco <jcfaracco@gmail.com>
>> ---
>>  src/storage/storage_driver.c | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>>
> 
> Although I see the point of the referenced bz - the bz and patch goes
> against the documented behavior, see:
> 
> https://libvirt.org/formatstorage.html
> 
> ...
> Storage volume XML
> ...
> Target elements
> ...
> permissions
> ...
> 
> "The mode element contains the octal permission set. The mode defaults
> to 0600 when not provided. The owner element contains the numeric user
> ID. The group element contains the numeric group ID. If owner or group
> aren't specified when creating a supported volume, the values are
> inherited from the parent directory."
> 

Actually it looks like these docs are wrong too, in my testing new
volumes do not inherit uid/gid from the parent pool, at least not for a
directory pool. I wrote those docs but I'm not sure if they were ever
correct, maybe it was wishful thinking?

Indeed I just ran v1.2.16 from danpb's virt-ark repo, set up a
qemu:///system dir pool owned by crobinso:crobinso, but by default
volumes are created with root:root, basically the user libvirtd is
running as. I'll send a patch to correct it

> Check out commit 7c2d65dd and fafcc818f for doc and code changes which 
> are the source of today's logic. If you follow those changes, you'll see
> that creation of pool and volume has always had some sort of default;
> however, determining when that default was provided or when perhaps user
> provided that same value is impossible. So, the usage of -1 for fields
> was done in order to help make that determination (for output purposes).
> 
> If you did change to use this default, then what would that do to the
> existing code using VIR_STORAGE_DEFAULT_VOL_PERM_MODE (O600)? Or how
> would code distinguish between something a consumer set and what is/was
> set "by default".
> 
> BTW: If you took using a storage pool permissions to be the permissions
> for the volume, then you'd need to consider that the default storage
> pool permissions is handled via VIR_STORAGE_DEFAULT_POOL_PERM_MODE
> (O711) which I hope you can agree/understand why you may not want that
> as your storage volume value (e.g. executable).
> 
> In the long run, defaults are defaults and if someone wants something
> other than the default, then they have the capability via input XML to
> change those defaults.

I agree, at this point changing the defaults could surprise some people
and the current behavior hasn't generated many complaints, so best to
just leave it as is. Apps could override this behavior easily enough if
they wanted, by copying uid/gid out of the parent XML and including it
in the new volume XML.

- Cole

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