On 02/10/2014 12:32 AM, Leonardo Garcia wrote:

From: Leonardo Garcia <lagarcia@br.ibm.com>

Just add a commit description

Signed-off-by: Leonardo Garcia <lagarcia@br.ibm.com> --- src/kimchi/utils.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/kimchi/utils.py b/src/kimchi/utils.py index 8795957..9368331 100644 --- a/src/kimchi/utils.py +++ b/src/kimchi/utils.py @@ -188,11 +188,11 @@ def run_command(cmd, timeout=None): timer.cancel()

-def parse_cmd_output(output, output_items): +def parse_cmd_output(output, output_items, separator=None): res = [] for line in output.split("\n"): if line: - res.append(dict(zip(output_items, line.split()))) + res.append(dict(zip(output_items, line.split(separator)))) return res

Reviewed-by: Daniel Barboza <danielhb@linux.vnet.ibm.com> On 02/10/2014 12:32 AM, Leonardo Garcia wrote:

From: Leonardo Garcia <lagarcia@br.ibm.com>

Remove useless statements and improve debug message.

Signed-off-by: Leonardo Garcia <lagarcia@br.ibm.com> --- src/kimchi/auth.py | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/src/kimchi/auth.py b/src/kimchi/auth.py index 242fdcf..f9873ca 100644 --- a/src/kimchi/auth.py +++ b/src/kimchi/auth.py @@ -87,13 +87,11 @@ def check_auth_session(): for the user. """ try: - s = cherrypy.session[SESSION_USER] - user = cherrypy.request.login = cherrypy.session[SESSION_USER] - debug("Authenticated with session: %s, for user: %s" % (s, user)) + user = cherrypy.session[USER_ID] + debug("Session authenticated for user %s" % user) except KeyError: debug("Session not found") return False - debug("Session found for user %s" % user) return True

@@ -135,8 +133,7 @@ def login(userid, password):

def logout(): cherrypy.session.acquire_lock() - userid = cherrypy.session.get(SESSION_USER, None) - cherrypy.session[SESSION_USER] = cherrypy.request.login = None + cherrypy.session[USER_ID] = None cherrypy.session.release_lock() cherrypy.lib.sessions.expire()

On 02/10/2014 10:32 AM, Leonardo Garcia wrote:

From: Leonardo Garcia <lagarcia@br.ibm.com>

Remove useless statements and improve debug message.

Signed-off-by: Leonardo Garcia <lagarcia@br.ibm.com> --- src/kimchi/auth.py | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/src/kimchi/auth.py b/src/kimchi/auth.py index 242fdcf..f9873ca 100644 --- a/src/kimchi/auth.py +++ b/src/kimchi/auth.py @@ -87,13 +87,11 @@ def check_auth_session(): for the user. """ try: - s = cherrypy.session[SESSION_USER] - user = cherrypy.request.login = cherrypy.session[SESSION_USER] - debug("Authenticated with session: %s, for user: %s" % (s, user)) + user = cherrypy.session[USER_ID] It seems USER_ID is defined in the following up patch in this series, so it needs a rebase. + debug("Session authenticated for user %s" % user) except KeyError: debug("Session not found") return False - debug("Session found for user %s" % user) return True

@@ -135,8 +133,7 @@ def login(userid, password):

def logout(): cherrypy.session.acquire_lock() - userid = cherrypy.session.get(SESSION_USER, None) - cherrypy.session[SESSION_USER] = cherrypy.request.login = None + cherrypy.session[USER_ID] = None we reset the session var to None on logout, but in check_auth_session, we check if the key exists. So check_auth_session will always hold true even after logout. @shaohe, it's a but, correct?

I know this problem exists in the original patch, but I think we should not keep the bug when it's changed.

cherrypy.session.release_lock() cherrypy.lib.sessions.expire()

a minor comment below On 02/10/2014 10:32 AM, Leonardo Garcia wrote:

From: Leonardo Garcia <lagarcia@br.ibm.com>

When the /login REST API is called with a valid username and password, Kimchi will find out the user groups and sudo status, store these values in sessions variables, and return them in a JSON message with the following format:

{"sudo": true | false, "userid": "<username>", "groups": [<list of groups>]}

For example:

{"sudo": false, "userid": "guest", "groups": ["guest", "foo"]}

This JSON return message can be used by the UI to figure out which tabs/options should be shown in the web interface according to the user rights.

It is important to notice that sudo configuration for a given user is not as simple as an on/off key. Sudo permits various levels of access controls. For instance, it is possible to specify which executables a user can run with sudo. For the purpose of this implementation, we only consider a user to have sudo rights if this user can run any command in the system with sudo (which is equivalent to have root rights).

Signed-off-by: Leonardo Garcia <lagarcia@br.ibm.com> --- src/kimchi/auth.py | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++---- src/kimchi/root.py | 4 ++-- 2 files changed, 58 insertions(+), 6 deletions(-)

diff --git a/src/kimchi/auth.py b/src/kimchi/auth.py index f9873ca..3ffe4b1 100644 --- a/src/kimchi/auth.py +++ b/src/kimchi/auth.py @@ -30,9 +30,12 @@ import re

from kimchi import template from kimchi.exception import OperationFailed +from kimchi.utils import run_command, parse_cmd_output

-SESSION_USER = 'userid' +USER_ID = 'userid' +USER_GROUPS = 'groups' +USER_SUDO = 'sudo'

def debug(msg): @@ -40,6 +43,52 @@ def debug(msg): # cherrypy.log.error(msg)

+class User(object): + + def __init__(self, userid): + self.user = {} + self.user[USER_ID] = userid should userid -> username? + self.user[USER_GROUPS] = [] + self.user[USER_SUDO] = False + + def get_groups(self): + out, err, exit = run_command(['groups', self.user[USER_ID]]) + # the output of the groups command is of the form: + # <userid> : <list of groups separated by space> + if exit == 0: + out = parse_cmd_output(out, ['userid', 'groups'], ':') + self.user[USER_GROUPS] = out[0]['groups'].split() + debug("Groups for %s: %s" % (self.user[USER_ID], + self.user[USER_GROUPS])) + return self.user[USER_GROUPS] + How about ?

+import grp + def get_groups(self): + return [g.gr_name for g in grp.getgrall() if user in g.gr_mem]

+ def has_sudo(self): + out, err, exit = run_command(['sudo', '-l', '-U', self.user[USER_ID], + 'sudo']) + if exit == 0: + debug("User %s is allowed to run sudo" % self.user[USER_ID]) + # sudo allows a wide range of configurations, such as controlling + # which binaries the user can execute with sudo. + # For now, we will just check whether the user is allowed to run any + # command with sudo. + out, err, exit = run_command(['sudo', '-l', '-U', + self.user[USER_ID]]) + for line in out.split('\n'): + if line and re.search("(ALL)", line):

(ALL) is OK for sudo? sometimes, even a user in sudo list, but it can not run all commands.

+ self.user[USER_SUDO] = True + debug("User %s can run any command with sudo" % + self.user[USER_ID]) + return self.user[USER_SUDO] + debug("User %s can only run some commands with sudo" % + self.user[USER_ID]) + else: + debug("User %s is not allowed to run sudo" % self.user[USER_ID]) + return self.user[USER_SUDO] + + def get_user(self): + return self.user + + def authenticate(username, password, service="passwd"): '''Returns True if authenticate is OK via PAM.''' def _pam_conv(auth, query_list, userData=None): @@ -122,13 +171,16 @@ def check_auth_httpba(): def login(userid, password): if not authenticate(userid, password): debug("User cannot be verified with the supplied password") - return False + return None + user = User(userid) debug("User verified, establishing session") cherrypy.session.acquire_lock() cherrypy.session.regenerate() - cherrypy.session[SESSION_USER] = cherrypy.request.login = userid + cherrypy.session[USER_ID] = userid + cherrypy.session[USER_GROUPS] = user.get_groups() + cherrypy.session[USER_SUDO] = user.has_sudo() cherrypy.session.release_lock() - return True + return user.get_user()

def logout(): diff --git a/src/kimchi/root.py b/src/kimchi/root.py index ce4a49c..8e90b7f 100644 --- a/src/kimchi/root.py +++ b/src/kimchi/root.py @@ -106,11 +106,11 @@ class KimchiRoot(Root): raise cherrypy.HTTPError(400, "Missing parameter: '%s'" % key)

try: - auth.login(userid, password) + user_info = auth.login(userid, password) except OperationFailed: raise cherrypy.HTTPError(401)

- return '{}' + return json.dumps(user_info)

@cherrypy.expose def logout(self):

-- Thanks and best regards! Sheldon Feng(冯少合)<shaohef@linux.vnet.ibm.com> IBM Linux Technology Center

On 02/10/2014 12:32 AM, Leonardo Garcia wrote:

From: Leonardo Garcia <lagarcia@br.ibm.com>

When the /login REST API is called with a valid username and password, Kimchi will find out the user groups and sudo status, store these values in sessions variables, and return them in a JSON message with the following format:

{"sudo": true | false, "userid": "<username>", "groups": [<list of groups>]}

For example:

{"sudo": false, "userid": "guest", "groups": ["guest", "foo"]}

This JSON return message can be used by the UI to figure out which tabs/options should be shown in the web interface according to the user rights.

It is important to notice that sudo configuration for a given user is not as simple as an on/off key. Sudo permits various levels of access controls. For instance, it is possible to specify which executables a user can run with sudo. For the purpose of this implementation, we only consider a user to have sudo rights if this user can run any command in the system with sudo (which is equivalent to have root rights).

Signed-off-by: Leonardo Garcia <lagarcia@br.ibm.com> --- src/kimchi/auth.py | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++---- src/kimchi/root.py | 4 ++-- 2 files changed, 58 insertions(+), 6 deletions(-)

diff --git a/src/kimchi/auth.py b/src/kimchi/auth.py index f9873ca..3ffe4b1 100644 --- a/src/kimchi/auth.py +++ b/src/kimchi/auth.py @@ -30,9 +30,12 @@ import re

from kimchi import template from kimchi.exception import OperationFailed +from kimchi.utils import run_command, parse_cmd_output

-SESSION_USER = 'userid' +USER_ID = 'userid' +USER_GROUPS = 'groups' +USER_SUDO = 'sudo'

def debug(msg): @@ -40,6 +43,52 @@ def debug(msg): # cherrypy.log.error(msg)

+class User(object): + + def __init__(self, userid): + self.user = {} + self.user[USER_ID] = userid + self.user[USER_GROUPS] = [] + self.user[USER_SUDO] = False +

+ def get_groups(self): + out, err, exit = run_command(['groups', self.user[USER_ID]]) + # the output of the groups command is of the form: + # <userid> : <list of groups separated by space> + if exit == 0: + out = parse_cmd_output(out, ['userid', 'groups'], ':') + self.user[USER_GROUPS] = out[0]['groups'].split() + debug("Groups for %s: %s" % (self.user[USER_ID], + self.user[USER_GROUPS])) + return self.user[USER_GROUPS]

You can use grp module here. http://stackoverflow.com/questions/9323834/python-how-to-get-group-ids-of-on...

+ + def has_sudo(self): + out, err, exit = run_command(['sudo', '-l', '-U', self.user[USER_ID], + 'sudo']) + if exit == 0: + debug("User %s is allowed to run sudo" % self.user[USER_ID]) + # sudo allows a wide range of configurations, such as controlling + # which binaries the user can execute with sudo. + # For now, we will just check whether the user is allowed to run any + # command with sudo. + out, err, exit = run_command(['sudo', '-l', '-U', + self.user[USER_ID]]) + for line in out.split('\n'): + if line and re.search("(ALL)", line): + self.user[USER_SUDO] = True + debug("User %s can run any command with sudo" % + self.user[USER_ID]) + return self.user[USER_SUDO] + debug("User %s can only run some commands with sudo" % + self.user[USER_ID]) + else: + debug("User %s is not allowed to run sudo" % self.user[USER_ID]) + return self.user[USER_SUDO] + + def get_user(self): + return self.user + + def authenticate(username, password, service="passwd"): '''Returns True if authenticate is OK via PAM.''' def _pam_conv(auth, query_list, userData=None): @@ -122,13 +171,16 @@ def check_auth_httpba(): def login(userid, password): if not authenticate(userid, password): debug("User cannot be verified with the supplied password") - return False + return None + user = User(userid) debug("User verified, establishing session") cherrypy.session.acquire_lock() cherrypy.session.regenerate() - cherrypy.session[SESSION_USER] = cherrypy.request.login = userid + cherrypy.session[USER_ID] = userid + cherrypy.session[USER_GROUPS] = user.get_groups() + cherrypy.session[USER_SUDO] = user.has_sudo() cherrypy.session.release_lock() - return True + return user.get_user()

def logout(): diff --git a/src/kimchi/root.py b/src/kimchi/root.py index ce4a49c..8e90b7f 100644 --- a/src/kimchi/root.py +++ b/src/kimchi/root.py @@ -106,11 +106,11 @@ class KimchiRoot(Root): raise cherrypy.HTTPError(400, "Missing parameter: '%s'" % key)

try: - auth.login(userid, password) + user_info = auth.login(userid, password) except OperationFailed: raise cherrypy.HTTPError(401)

- return '{}' + return json.dumps(user_info)

@cherrypy.expose def logout(self):

Reviewed-by: Daniel Barboza <danielhb@linux.vnet.ibm.com> On 02/10/2014 12:32 AM, Leonardo Garcia wrote:

From: Leonardo Garcia <lagarcia@br.ibm.com>

kimchiauth tool used to only check if the user was authenticated or not. Now it also checks whether the REST API being accessed is only allowed to users with sudo rights.

The necessity to have sudo rights to access a REST API can be easily configured through the UrlSubNode decorator. Similar to the support previously implemented for user authentication in UrlSubNode, an additional boolean parameter was added to UrlSubNode to indicate whether the user needs sudo rights in order to access the corresponding REST API.

Signed-off-by: Leonardo Garcia <lagarcia@br.ibm.com> --- src/kimchi/auth.py | 10 +++++++--- src/kimchi/control/utils.py | 4 +++- src/kimchi/server.py | 2 ++ 3 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/src/kimchi/auth.py b/src/kimchi/auth.py index 3ffe4b1..b3d1edf 100644 --- a/src/kimchi/auth.py +++ b/src/kimchi/auth.py @@ -190,12 +190,16 @@ def logout(): cherrypy.lib.sessions.expire()

-def kimchiauth(*args, **kwargs): +def kimchiauth(needs_admin=False): debug("Entering kimchiauth...") - if check_auth_session(): + if check_auth_session() and \ + (not needs_admin or (cherrypy.session[USER_SUDO] == needs_admin)): + debug(str(cherrypy.session[USER_SUDO])) return

- if check_auth_httpba(): + if check_auth_httpba() and \ + (not needs_admin or (cherrypy.session[USER_SUDO] == needs_admin)): + debug(str(cherrypy.session[USER_SUDO])) return

if not from_browser(): diff --git a/src/kimchi/control/utils.py b/src/kimchi/control/utils.py index 9c6878b..4567af7 100644 --- a/src/kimchi/control/utils.py +++ b/src/kimchi/control/utils.py @@ -107,13 +107,15 @@ def validate_params(params, instance, action):

class UrlSubNode(object): - def __init__(self, name, auth=False): + def __init__(self, name, auth=False, needs_admin=False): self.name = name self.auth = auth + self.needs_admin = needs_admin

def __call__(self, fun): fun._url_sub_node_name = {"name": self.name} fun.url_auth = self.auth + fun.needs_admin = self.needs_admin return fun

diff --git a/src/kimchi/server.py b/src/kimchi/server.py index 1e131b4..469db68 100644 --- a/src/kimchi/server.py +++ b/src/kimchi/server.py @@ -191,6 +191,8 @@ class Server(object): for ident, node in sub_nodes.items(): if node.url_auth: self.configObj["/%s" % ident] = {'tools.kimchiauth.on': True} + if node.needs_admin: + self.configObj["/%s" % ident]['tools.kimchiauth.needs_admin'] = True

self.app = cherrypy.tree.mount(KimchiRoot(model_instance, dev_env), config=self.configObj)

On 02/10/2014 05:19 PM, Aline Manera wrote:

On 02/10/2014 12:32 AM, Leonardo Garcia wrote:

...

From: Leonardo Garcia <lagarcia@br.ibm.com>

kimchiauth tool used to only check if the user was authenticated or not. Now it also checks whether the REST API being accessed is only allowed to users with sudo rights.

The necessity to have sudo rights to access a REST API can be easily configured through the UrlSubNode decorator. Similar to the support previously implemented for user authentication in UrlSubNode, an additional boolean parameter was added to UrlSubNode to indicate whether the user needs sudo rights in order to access the corresponding REST API.

Signed-off-by: Leonardo Garcia <lagarcia@br.ibm.com> --- src/kimchi/auth.py | 10 +++++++--- src/kimchi/control/utils.py | 4 +++- src/kimchi/server.py | 2 ++ 3 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/src/kimchi/auth.py b/src/kimchi/auth.py index 3ffe4b1..b3d1edf 100644 --- a/src/kimchi/auth.py +++ b/src/kimchi/auth.py @@ -190,12 +190,16 @@ def logout(): cherrypy.lib.sessions.expire()

-def kimchiauth(*args, **kwargs): +def kimchiauth(needs_admin=False): debug("Entering kimchiauth...") - if check_auth_session(): + if check_auth_session() and \ + (not needs_admin or (cherrypy.session[USER_SUDO] == needs_admin)): + debug(str(cherrypy.session[USER_SUDO])) return

- if check_auth_httpba(): + if check_auth_httpba() and \ + (not needs_admin or (cherrypy.session[USER_SUDO] == needs_admin)): + debug(str(cherrypy.session[USER_SUDO])) return

if not from_browser(): diff --git a/src/kimchi/control/utils.py b/src/kimchi/control/utils.py index 9c6878b..4567af7 100644 --- a/src/kimchi/control/utils.py +++ b/src/kimchi/control/utils.py @@ -107,13 +107,15 @@ def validate_params(params, instance, action):

class UrlSubNode(object): - def __init__(self, name, auth=False): + def __init__(self, name, auth=False, needs_admin=False):

We also need to have a list of which methods are exclusive for admin For example, any kind of user can do GET operations, but POST, PUT and DELETE are only available for admin

def __init__(self, name, auth=False, needs_admin=False, admin_methods=[]) fun.admin_methods = admin_methods

And in kimchiauth()

method = cherrypy.request.method.upper() if method in [admin_methods]: # needs sudo

Or instead of pass admin_methods() we assume in kimchiauth() only GET method does not require admin access.

...

self.name = name self.auth = auth + self.needs_admin = needs_admin

def __call__(self, fun): fun._url_sub_node_name = {"name": self.name} fun.url_auth = self.auth + fun.needs_admin = self.needs_admin return fun

diff --git a/src/kimchi/server.py b/src/kimchi/server.py index 1e131b4..469db68 100644 --- a/src/kimchi/server.py +++ b/src/kimchi/server.py @@ -191,6 +191,8 @@ class Server(object): for ident, node in sub_nodes.items(): if node.url_auth: self.configObj["/%s" % ident] = {'tools.kimchiauth.on': True} + if node.needs_admin: + self.configObj["/%s" % ident]['tools.kimchiauth.needs_admin'] = True

self.app = cherrypy.tree.mount(KimchiRoot(model_instance, dev_env), config=self.configObj)

_______________________________________________ Kimchi-devel mailing list Kimchi-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel

On 02/10/2014 05:23 PM, Aline Manera wrote:

...

On 02/10/2014 05:19 PM, Aline Manera wrote:

...

On 02/10/2014 12:32 AM, Leonardo Garcia wrote:

...

From: Leonardo Garcia <lagarcia@br.ibm.com>

kimchiauth tool used to only check if the user was authenticated or not. Now it also checks whether the REST API being accessed is only allowed to users with sudo rights.

The necessity to have sudo rights to access a REST API can be easily configured through the UrlSubNode decorator. Similar to the support previously implemented for user authentication in UrlSubNode, an additional boolean parameter was added to UrlSubNode to indicate whether the user needs sudo rights in order to access the corresponding REST API.

Signed-off-by: Leonardo Garcia <lagarcia@br.ibm.com> --- src/kimchi/auth.py | 10 +++++++--- src/kimchi/control/utils.py | 4 +++- src/kimchi/server.py | 2 ++ 3 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/src/kimchi/auth.py b/src/kimchi/auth.py index 3ffe4b1..b3d1edf 100644 --- a/src/kimchi/auth.py +++ b/src/kimchi/auth.py @@ -190,12 +190,16 @@ def logout(): cherrypy.lib.sessions.expire()

-def kimchiauth(*args, **kwargs): +def kimchiauth(needs_admin=False): debug("Entering kimchiauth...") - if check_auth_session(): + if check_auth_session() and \ + (not needs_admin or (cherrypy.session[USER_SUDO] == needs_admin)): + debug(str(cherrypy.session[USER_SUDO])) return

- if check_auth_httpba(): + if check_auth_httpba() and \ + (not needs_admin or (cherrypy.session[USER_SUDO] == needs_admin)): + debug(str(cherrypy.session[USER_SUDO])) return

if not from_browser(): diff --git a/src/kimchi/control/utils.py b/src/kimchi/control/utils.py index 9c6878b..4567af7 100644 --- a/src/kimchi/control/utils.py +++ b/src/kimchi/control/utils.py @@ -107,13 +107,15 @@ def validate_params(params, instance, action):

class UrlSubNode(object): - def __init__(self, name, auth=False): + def __init__(self, name, auth=False, needs_admin=False): We also need to have a list of which methods are exclusive for admin For example, any kind of user can do GET operations, but POST, PUT and DELETE are only available for admin

def __init__(self, name, auth=False, needs_admin=False, admin_methods=[]) fun.admin_methods = admin_methods

And in kimchiauth()

method = cherrypy.request.method.upper() if method in [admin_methods]: # needs sudo

Or instead of pass admin_methods() we assume in kimchiauth() only GET method does not require admin access. Yes, this is a better approach, definitely. Hmmm... I've been thinking about this and I think that it is wrong to assume that GET method will not require admin access at all times. I

On 02/11/2014 01:17 AM, Leonardo Augusto Guimarães Garcia wrote: think there might be some REST APIs for which no even GET should be permitted for non-admin user. So, I'll send v2 with a patch similar to what you proposed above, but with only one argument: needs_admin will be a list of methods that need admin rights to be accessed. Best regards, Leonardo Garcia

I'll include this check in v2.

Best regards,

Leonardo Garcia

...

...

...

self.name = name self.auth = auth + self.needs_admin = needs_admin

def __call__(self, fun): fun._url_sub_node_name = {"name": self.name} fun.url_auth = self.auth + fun.needs_admin = self.needs_admin return fun

diff --git a/src/kimchi/server.py b/src/kimchi/server.py index 1e131b4..469db68 100644 --- a/src/kimchi/server.py +++ b/src/kimchi/server.py @@ -191,6 +191,8 @@ class Server(object): for ident, node in sub_nodes.items(): if node.url_auth: self.configObj["/%s" % ident] = {'tools.kimchiauth.on': True} + if node.needs_admin: + self.configObj["/%s" % ident]['tools.kimchiauth.needs_admin'] = True

self.app = cherrypy.tree.mount(KimchiRoot(model_instance, dev_env), config=self.configObj)

_______________________________________________ Kimchi-devel mailing list Kimchi-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel

---

Kimchi-devel mailing list Kimchi-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/kimchi-devel