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

[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 *