From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 77E7826895 for ; Wed, 11 Jul 2018 14:34:01 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id Mjh6mJ3B3qiv for ; Wed, 11 Jul 2018 14:34:01 -0400 (EDT) Received: from smtp37.i.mail.ru (smtp37.i.mail.ru [94.100.177.97]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id AD134267A6 for ; Wed, 11 Jul 2018 14:34:00 -0400 (EDT) Date: Wed, 11 Jul 2018 21:33:58 +0300 From: Konstantin Osipov Subject: [tarantool-patches] Re: [PATCH] Make access_check_ddl check for entity privileges. Message-ID: <20180711183358.GA31667@chai> References: <20180711164036.33604-1-sergepetrenko@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180711164036.33604-1-sergepetrenko@tarantool.org> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org Cc: Serge Petrenko * Serge Petrenko [18/07/11 19:42]: > /* > * Only the owner of the object or someone who has > * specific DDL privilege on the object can execute > @@ -96,6 +102,40 @@ access_check_ddl(const char *name, uint32_t owner_uid, > */ > if (access == 0 || (is_owner && !(access & (PRIV_U|PRIV_C)))) > return; /* Access granted. */ > + int rc = -1; > + if (!(access & (PRIV_U | PRIV_C))) { You can't grant CREATE privilege to a non-existing object. USAGE can be granted only globally. This comment explains this if statement. Please add it. I > + * Ignore universe and unknown > + * types here, since universe is already handled, and what > + * to do with unknown is unknown. I don't understand this comment, please rephrase or remove. > + * > + * Currently no specific privileges to a single role, user, > + * collation. > + */ > + switch (type) { > + case SC_SPACE: > + rc = access_check_space( > + space_cache_find_xc(object_id), > + access); > + break; This switch statement is not aligned right. You should use lisp-style alignment for function arguments. Throwing ER_NO_SUCH_SPACE is a security breach, you should not reveal that the space does not exist, instead simply say that the user has no requested access to space (ER_ACCESS_DENIED). > + case SC_SEQUENCE: > + if (priv_type == PRIV_W) { > + rc = access_check_sequence( > + sequence_cache_find(object_id)); > + break; > + } Wrong alignment, same argument about exceptions. > + case SC_FUNCTION: > + case SC_USER: > + case SC_ROLE: > + case SC_COLLATION: What about these? Can't you grant DROP/ALTER on a specific user or role? > @@ -1751,11 +1791,9 @@ on_replace_dd_index(struct trigger * /* trigger */, void *event) > uint32_t iid = tuple_field_u32_xc(old_tuple ? old_tuple : new_tuple, > BOX_INDEX_FIELD_ID); > struct space *old_space = space_cache_find_xc(id); > - enum priv_type priv_type = new_tuple ? PRIV_C : PRIV_D; > - if (old_tuple && new_tuple) > - priv_type = PRIV_A; > - access_check_ddl(old_space->def->name, old_space->def->uid, SC_SPACE, > - priv_type, true); > + enum priv_type priv_type = new_tuple ? PRIV_A : PRIV_D; > + access_check_ddl(old_space->def->name, old_space->def->id, > + old_space->def->uid, SC_SPACE, priv_type, true); As far as I understand, you changed it because creating an index is technically altering a space, not creating it. But in this case dropping an index is also technically altering a space. In SQL, CREATE/DROP/ALTER match SQL statements CREATE/DROP/ALTER respectively. Since in NoSQL in Tarantool we don't have these statements, instead, we create each index with a separate Lua command, let's keep the old check: use CREATE access to space in order to permit CREATING an index, ALTER - to permit update, and DROP - to permit drop. > @@ -2185,8 +2224,8 @@ on_replace_dd_user(struct trigger * /* trigger */, void *event) > * correct. > */ > struct user_def *user = user_def_new_from_tuple(new_tuple); > - access_check_ddl(user->name, user->uid, SC_USER, PRIV_A, > - true); > + access_check_ddl(user->name, user->uid, user->uid, SC_USER, > + PRIV_A, true); Why did you write user->uid here for the owner? > end > + if object_type == 'user' then > + if object_name == nil or object_name == 0 then > + return 0 > + end > + -- otherwise some error. Don't know which one yet. > + box.error(box.error.NO_SUCH_USER, object_name) > + end It would be better if you extract new entities into a separate patch. The bigger the patch gets the less changes for it to get in - there always is something that can be improved. -- Konstantin Osipov, Moscow, Russia, +7 903 626 22 32 http://tarantool.io - www.twitter.com/kostja_osipov