[pve-devel] [PATCH] implement chown and chmod for user root group www-data and perm 0640

[Stefan Priebe - Profihost AG]

Hi Thomas,

thanks and yes if you will do a V5 it would be great!

Stefan

Am 21.03.2017 um 10:46 schrieb Thomas Lamprecht:
> Hi,
> 
> On 03/20/2017 03:11 PM, Stefan Priebe wrote:
>> This allows us to use management software for files inside of /etc/pve.
>> f.e. saltstack which rely on being able to set uid,gid and chmod
>>
>> Reviewed-by: Thomas Lamprecht <t.lamprecht at proxmox.com>
> 
> If you change the patch behavior (comments or other non semantic changes
> are normally fine) without previous discussion with the old reviewed-by
> people
> I'd suggest dropping them when sending it, as else it suggest that the
> new behavior
> and code was also already reviewed. I know I'm nitpicking for such a
> "small" change,
> but the cluster filesystem is a core part of ours and permission should
> work.
> Anyways, would be great for the next time :)
> 
>> Signed-off-by: Stefan Priebe <s.priebe at profihost.ag>
>> ---
>>   data/src/pmxcfs.c | 38 +++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 37 insertions(+), 1 deletion(-)
>>
>> diff --git a/data/src/pmxcfs.c b/data/src/pmxcfs.c
>> index 1b6cbcc..5f45115 100644
>> --- a/data/src/pmxcfs.c
>> +++ b/data/src/pmxcfs.c
>> @@ -186,6 +186,40 @@ ret:
>>       return ret;
>>   }
>>   +static int cfs_fuse_chmod(const char *path, mode_t mode)
>> +{
>> +    int ret = -EACCES;
>> +
>> +    cfs_debug("enter cfs_fuse_chmod %s", path);
>> +
>> +    // asserts 0640 or 0600, but allows setting UID and GID - some
>> programs need that
>> +    if (path_is_private(path)) {
>> +        if ((mode & ACCESSPERMS) == (S_IRUSR | S_IWUSR))
>> +            ret = 0;
>> +    } else if ((mode & ACCESSPERMS) == (S_IRUSR | S_IWUSR | S_IRGRP)) {
>> +        ret = 0;
>> +    }
>> +
> 
> With the new addition, may I propose another refactoring of above,
> as IMO nested if branches with no parenthesis in the inner level seems
> confusing.
> (I generally do not like the "no-parenthesis if one liners", but it
> seems they are
> widely used here, so I also do not want to break with the style.)
> 
> Whats your opinion on something like:
> 
> -- 
> static int cfs_fuse_chmod(const char *path, mode_t mode)
> {
>         int ret = -EACCES;
> 
>         cfs_debug("enter cfs_fuse_chmod %s", path);
> 
>         mode_t allowed_mode = (S_IRUSR | S_IWUSR);
>         if (!path_is_private(path))
>                 allowed_mode |= (S_IRGRP);
> 
>         // ACCESSPERMS masks out UID and GID for the check, some
> programs need to set them
>         if ((mode & ACCESSPERMS) == allowed_mode)
>                 ret = 0;
> 
>         cfs_debug("leave cfs_fuse_chmod %s (%d) mode: %o", path, ret,
> (int)mode);
> 
>         return ret;
> }
> -- 
> 
> Else, all looks good. I can also make a V5 with those changes on top of
> yours, if you prefer.
> 
> cheers,
> Thomas
> 
>> +    cfs_debug("leave cfs_fuse_chmod %s (%d) mode: %o", path, ret,
>> (int)mode);
>> +
>> +    return ret;
>> +}
>> +
>> +static int cfs_fuse_chown(const char *path, uid_t user, gid_t group)
>> +{
>> +    int ret = -EACCES;
>> +
>> +    cfs_debug("enter cfs_fuse_chown %s", path);
>> +
>> +    // we get -1 if no change should be made
>> +    if ((user == 0 || user == -1) && (group == cfs.gid || group == -1))
>> +        ret = 0;
>> +
>> +    cfs_debug("leave cfs_fuse_chown %s (%d) (uid: %d; gid: %d)",
>> path, ret, user, group);
>> +
>> +    return ret;
>> +}
>> +
>>   static int cfs_fuse_mkdir(const char *path, mode_t mode)
>>   {
>>       cfs_debug("enter cfs_fuse_mkdir %s", path);
>> @@ -488,7 +522,9 @@ static struct fuse_operations fuse_ops = {
>>       .readlink = cfs_fuse_readlink,
>>       .utimens = cfs_fuse_utimens,
>>       .statfs = cfs_fuse_statfs,
>> -    .init = cfs_fuse_init
>> +    .init = cfs_fuse_init,
>> +    .chown = cfs_fuse_chown,
>> +    .chmod = cfs_fuse_chmod
>>   };
>>     static char *
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> http://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel