[Kimchi-devel] [PATCH V5] add a method to fix search permissions

Tue Jan 7 19:05:06 UTC 2014

On 01/07/2014 05:51 AM, Sheldon wrote:
> On 01/07/2014 03:15 AM, Aline Manera wrote:
>> On 01/06/2014 01:55 PM, shaohef at linux.vnet.ibm.com wrote:
>>> From: ShaoHe Feng <shaohef at linux.vnet.ibm.com>
>>>
>>> First will use setfacl to set the path search permission for user or
>>> group, if failed then set search permission for others on the path.
>>>
>>> Usage:
>>> check_path_permission("/tmp/need/to/fix/path", username)
>>> fix_path_permission("/tmp/need/to/fix/path", username)
>>>
>>> Signed-off-by: ShaoHe Feng <shaohef at linux.vnet.ibm.com>
>>> Signed-off-by: Royce Lv <lvroyce at linux.vnet.ibm.com>
>>> ---
>>> Makefile.am | 1 +
>>> src/kimchi/utils.py | 125 
>>> +++++++++++++++++++++++++++++++++++++++++++++++++++-
>>> 2 files changed, 125 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/Makefile.am b/Makefile.am
>>> index 07b2aa0..bf8e0a0 100644
>>> --- a/Makefile.am
>>> +++ b/Makefile.am
>>> @@ -46,6 +46,7 @@ PEP8_WHITELIST = \
>>> src/kimchi/cachebust.py \
>>> src/kimchi/config.py.in \
>>> src/kimchi/control/*.py \
>>> + src/kimchi/utils.py \
>>> src/kimchi/disks.py \
>>> src/kimchi/featuretests.py \
>>> src/kimchi/iscsi.py \
>>> diff --git a/src/kimchi/utils.py b/src/kimchi/utils.py
>>> index b6d84fd..5de982f 100644
>>> --- a/src/kimchi/utils.py
>>> +++ b/src/kimchi/utils.py
>>> @@ -18,11 +18,16 @@
>>> #
>>> # You should have received a copy of the GNU Lesser General Public
>>> # License along with this library; if not, write to the Free Software
>>> -# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 
>>> 02110-1301 USA
>>> +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 
>>> 02110-1301 USA
>>> #
>>>
>>> import cherrypy
>>> +import grp
>>> import os
>>> +import pwd
>>> +import re
>>> +import stat
>>> +import subprocess
>>>
>>>
>>> from cherrypy.lib.reprconf import Parser
>>> @@ -33,6 +38,7 @@ from kimchi import config
>>>
>>> kimchi_log = cherrypy.log.error_log
>>>
>>> +
>>> def is_digit(value):
>>> if isinstance(value, int):
>>> return True
>>> @@ -84,3 +90,120 @@ def import_class(class_path):
>>>
>>> def import_module(module_name):
>>> return __import__(module_name, globals(), locals(), [''])
>>> +
>>> +
>>> +def run_command(cmd):
>>> + try:
>>> + proc = subprocess.Popen(cmd, stdout=subprocess.PIPE,
>>> + stderr=subprocess.PIPE)
>>> + out, error = proc.communicate()
>>> +
>>> + kimchi_log.debug("Run command '%s'", " ".join(cmd))
>>> + if out or error:
>>> + kimchi_log.debug("out:\n %s\nerror:\n %s", out, error)
>>> + except OSError as e:
>>> + kimchi_log.error("Failed to run command %s. %s", " ".join(cmd),
>>> + e.message)
>>> + return (None, None, None)
>>> + if proc.returncode != 0:
>>> + kimchi_log.debug("Cmd '%s' failed, error code: %s", cmd, error)
>>> + return (out, error, proc.returncode)
>>> +
>>> +
>>> +def name_uid(user):
>>> + return pwd.getpwnam(user).pw_uid
>>> +
>>> +
>>> +def get_group_ids(user):
>>> + gids = [g.gr_gid for g in grp.getgrall() if user in g.gr_mem]
>>> + gid = pwd.getpwnam(user).pw_gid
>>> + gids.append(grp.getgrgid(gid).gr_gid)
>>> + return gids
>>> +
>>> +
>>> +def get_groups(user):
>>> + gids = get_group_ids(user)
>>> + return [grp.getgrgid(gid).gr_name for gid in gids]
>>> +
>>> +
>>> +def run_setfacl_set_x(path, user="qemu", group=None):
>>
>> I think the correct default value for 'groups' is "" instead of None
>> Or we will run: setfacl --modify group:None:x <path>
> sure, "" is good. will change it in next version,.
>
> I have check in the follow code:
> + if ret != 0 and group:
> + out, error, ret = run_command(set_group)
>
> In my option, if we can set user ACL successful, do we still need to 
> set group ACL?

I don't think so. Only fixing the user ACL should be enough

>>
>>> + set_user = ["setfacl", "--modify", "user:%s:x" % user, path]
>>> + set_group = ["setfacl", "--modify", "group:%s:x" % group, path]
>>> + out, error, ret = run_command(set_user)
>>> + if ret != 0 and group:
>>> + out, error, ret = run_command(set_group)
>>> + return ret == 0
>>> +
>>> +
>>> +def run_getfacl(path, user="qemu", groups=None):
>>
>> Same I commented above about the default value for 'groups' parameter
>>
>>> + cmd = ["getfacl", path]
>>> + out, error, ret = run_command(cmd)
>>> + if out and ret == 0:
>>> + res = re.findall("user:%s:..x" % user, out)
>>> + if res:
>>> + return True
>>> + res = re.findall("(?<=group:)(.*)(?=:..x)",
>>> + out) if groups else []
>>> + return list(set(res) & set(groups)) != []
>>> + return False
>>> +
>>> +
>>> +def set_x_on_path(path, who="other"):
>>> + S_IXWHO = ((stat.S_IXUSR if who == "user" else None) or
>>> + (stat.S_IXGRP if who == "group" else None) or
>>> + stat.S_IXOTH)
>>> + mode = os.stat(path).st_mode | S_IXWHO
>>> + os.chmod(path, mode)
>>> + ret = os.stat(path).st_mode == mode
>>> + if not ret:
>>> + kimchi_log.debug("faild to set +x attribute on %s", path)
>>> + return ret
>>> +
>>> +
>>> +def get_x_on_path(path, uid, gids=[]):
>>> + try:
>>> + st = os.stat(path)
>>> + mode = st.st_mode
>>> + return ((uid == st.st_uid and mode & stat.S_IXUSR == stat.S_IXUSR) or
>>> + (st.st_gid in gids and mode & stat.S_IXGRP == stat.S_IXGRP) or
>>> + mode & stat.S_IXOTH == stat.S_IXOTH)
>>> + except OSError as e:
>>> + kimchi_log.error("faild to get attribute on %s as user id: %s. %s",
>>> + path, uid, e.message)
>>> + return False
>>> +
>>> +
>>
>>> +def reverse_paths(path):
>>> + paths = []
>>> + while path:
>>> + paths.append(path)
>>> + if path == "/":
>>> + break
>>> + path = os.path.dirname(path)
>>> +
>>> + paths.reverse()
>>> + return paths
>>> +
>>
>> As I commented before we can simplify it by using just one loop:
>>
>> paths = path.split("/")
>> paths = paths[1:]
>>
>> path = "/"
>> for p in paths:
>> path = os.path.join(path, p)
>> # check_path_permission code
>>
> will change in the next version.
>>> +
>>> +def check_path_permission(path, user='qemu', groups=[]):
>>> + groups = groups if groups else get_groups(user)
>>> + gids = [grp.getgrnam(group).gr_gid for group in groups] if groups 
>>> else []
>>> + paths = reverse_paths(path)
>>> + for path in paths:
>>> + if not (get_x_on_path(path, name_uid(user), gids) or
>>> + run_getfacl(path, user, groups)):
>>> + return False
>>> + return True
>>> +
>>> +
>>
>> We don't need this check_path_permission function
>> We can run fix_path_permission directly and it solves the problem 
>> when needed.
>>
>>> +def fix_path_permission(path, user='qemu', groups=[]):
>>> + groups = groups if groups else get_groups(user)
>>> + gids = [grp.getgrnam(group).gr_gid for group in groups] if groups 
>>> else []
>>> + paths = reverse_paths(path)
>>> + for path in paths:
>>> + if not (get_x_on_path(path, name_uid(user), gids) or
>>> + run_getfacl(path, user, groups) or
>>> + run_setfacl_set_x(path, user) or
>>> + set_x_on_path(path)):
>>> + return False
>>> + return True
>>
>>
>>
>
>