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 B71DB27691 for ; Thu, 26 Jul 2018 16:37:33 -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 YrQAoJlRti7E for ; Thu, 26 Jul 2018 16:37:33 -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 715AE2768C for ; Thu, 26 Jul 2018 16:37:33 -0400 (EDT) Date: Thu, 26 Jul 2018 23:37:31 +0300 From: Konstantin Osipov Subject: [tarantool-patches] Re: [PATCH v2 3/4] Add single object privilege checks to access_check_ddl. Message-ID: <20180726203731.GA32000@chai> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: 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/17 21:31]: This patch is generally LGTM, but it's based on entity access, so I can't push it, since I requested some changes in entity access patch. A few questions below. > access_check_ddl() didn't check for single object privileges, e.g. user > with alter access on a space couldn't create an index in this space. It > would only succeed if it had alter on entire entity space. > Fix this by adding single object privilege checks to access_check_ddl and > adding access cache to struct user, to hold other users' privileges on it. > > Also checking for single object privilege made it possible to grant > every user alter privilege on itself, so that a user may change its own > password (previously it was possible because of a hack). Removed the > hack, and added grant alter to itself upon user creation. > Modified tests accordingly, and added a couple of test cases. What hack did you remove? The problem I have with self-granting 'alter' is that old deployments will break, since they don't have a self-grant. Maybe we leave the hack in place for a while, begin self-granting 'alter', and only remove the hack in 2.0? We shouldn't forget to grant everyone an 'alter' on self in 2.0 upgrade script. But to begin with, I can't find where this hack was in the code, and where you removed it. > Closes #3530 > --- > src/box/alter.cc | 123 ++++++++++++++++++++++---------- > src/box/lua/schema.lua | 5 +- > src/box/user.cc | 10 ++- > src/box/user.h | 2 + > test/box/access.result | 182 +++++++++++++++++++++++++++++++++++++++++++++++ > test/box/access.test.lua | 53 ++++++++++++++ > test/box/role.result | 9 +++ > test/box/sequence.result | 3 + > 8 files changed, 347 insertions(+), 40 deletions(-) > > diff --git a/src/box/alter.cc b/src/box/alter.cc > index 6293dcc50..54a09664b 100644 > --- a/src/box/alter.cc > +++ b/src/box/alter.cc > @@ -62,7 +62,8 @@ > /* {{{ Auxiliary functions and methods. */ > > static void > -access_check_ddl(const char *name, uint32_t owner_uid, > +access_check_ddl(const char *name, uint32_t object_id, > + uint32_t owner_uid, > enum schema_object_type type, > enum priv_type priv_type, > bool is_17_compat_mode) > @@ -103,7 +104,48 @@ access_check_ddl(const char *name, uint32_t owner_uid, > */ > if (access == 0 || (is_owner && !(access & (PRIV_U | PRIV_C)))) > return; /* Access granted. */ > - > + /* > + * You can't grant CREATE privilege to a non-existing object. > + * USAGE can be granted only globally. > + */ > + if (!(access & (PRIV_U | PRIV_C))) { > + /* Check for privileges on a single object. */ Please avoid this deep nesting, use early returns from the function for example, or create a separate function, access_check_ddl_object()? > + switch (type) { > + case SC_SPACE: > + { > + struct space *space = space_by_id(object_id); > + if (space) > + access &= ~space->access[cr->auth_token].effective; > + break; > + } > + case SC_FUNCTION: > + { > + struct func *func = func_by_id(object_id); > + if (func) > + access &= ~func->access[cr->auth_token].effective; > + break; > + } > + case SC_USER: > + case SC_ROLE: > + { > + struct user *user_or_role = user_by_id(object_id); > + if (user_or_role) > + access &= ~user_or_role->access[cr->auth_token].effective; > + break; > + } > + case SC_SEQUENCE: > + { > + struct sequence *seq = sequence_by_id(object_id); > + if (seq) > + access &= ~seq->access[cr->auth_token].effective; > + break; > + } > + default: > + break; > + } OK. > * check that it has read and write access. > */ > - access_check_ddl(seq->def->name, seq->def->uid, SC_SEQUENCE, > - PRIV_R, false); > - access_check_ddl(seq->def->name, seq->def->uid, SC_SEQUENCE, > - PRIV_W, false); > + access_check_ddl(seq->def->name, seq->def->id, seq->def->uid, > + SC_SEQUENCE, PRIV_R, false); > + access_check_ddl(seq->def->name, seq->def->id, seq->def->uid, > + SC_SEQUENCE, PRIV_W, false); > } > /** Check we have alter access on space. */ > - access_check_ddl(space->def->name, space->def->uid, SC_SPACE, PRIV_A, > - false); > + access_check_ddl(space->def->name, space->def->id, space->def->uid, > + SC_SPACE, PRIV_A, false); > > struct trigger *on_commit = > txn_alter_trigger_new(on_commit_dd_space_sequence, space); > diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua > index 4b7a14411..a098e44fe 100644 > --- a/src/box/lua/schema.lua > +++ b/src/box/lua/schema.lua > @@ -1740,7 +1740,7 @@ local priv_object_combo = { > ["role"] = bit.bor(box.priv.X, box.priv.U, > box.priv.C, box.priv.D), > ["user"] = bit.bor(box.priv.C, box.priv.U, > - box.priv.D), > + box.priv.A, box.priv.D), > } > -- grant role 'public' to the user > box.schema.user.grant(uid, 'public') > + -- grant user 'alter' on itself, so it can > + -- change its password or username. > + box.schema.user.grant(uid, 'alter', 'user', uid) -- Konstantin Osipov, Moscow, Russia, +7 903 626 22 32 http://tarantool.io - www.twitter.com/kostja_osipov