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 ABC5B28041 for ; Mon, 30 Jul 2018 03:52:12 -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 kMp_rEFifq3d for ; Mon, 30 Jul 2018 03:52:12 -0400 (EDT) Received: from smtp38.i.mail.ru (smtp38.i.mail.ru [94.100.177.98]) (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 DA1572801D for ; Mon, 30 Jul 2018 03:52:11 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 11.5 \(3445.9.1\)) Subject: [tarantool-patches] Re: [PATCH] Add entities user, role to access control. From: Sergey Petrenko In-Reply-To: <20180726201552.GB26714@chai> Date: Mon, 30 Jul 2018 10:52:08 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: References: <20180712152932.60750-1-sergepetrenko@tarantool.org> <20180726201552.GB26714@chai> 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: Konstantin Osipov Cc: tarantool-patches@freelists.org Hi! I made a separate patch introducing new schema object types to be = used under the hood when granting privileges on an entire entity. I also = rebased this patch on top of the latter one and fixed your comments. Please see the = new diff below. > 26 =D0=B8=D1=8E=D0=BB=D1=8F 2018 =D0=B3., =D0=B2 23:15, Konstantin = Osipov =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0=D0=BB(=D0= =B0): >=20 > * Serge Petrenko [18/07/12 20:54]: >> + /* Do not allow changes for system users and roles. */ >> + struct credentials *cr =3D effective_user(); >> + if ((uid <=3D (uint32_t) BOX_SYSTEM_USER_ID_MAX || uid = =3D=3D SUPER) && >> + cr->uid !=3D ADMIN) { >> + struct user *current_user =3D = user_find_xc(cr->uid); >> + tnt_raise(AccessDeniedError, "alter", "user or = role", >> + old_user->def->name, = current_user->def->name); >> + } >=20 > It should be possible to change these system objects, provided the > person knows what they are doing, please remove this check, esp. > since it's outside the scope of this bug fix. Removed this check. >=20 >> case SC_ROLE: >> { >> - struct user *role =3D user_by_id(priv->object_id); >> - if (role =3D=3D NULL || role->def->type !=3D SC_ROLE) { >> - tnt_raise(ClientError, ER_NO_SUCH_ROLE, >> - role ? role->def->name : >> - int2str(priv->object_id)); >> - } >> - /* >> - * Only the creator of the role can grant or revoke it. >> - * Everyone can grant 'PUBLIC' role. >> - */ >> - if (role->def->owner !=3D grantor->def->uid && >> - grantor->def->uid !=3D ADMIN && >> - (role->def->uid !=3D PUBLIC || priv->access !=3D = PRIV_X)) { >> + if (priv->object_id !=3D 0) { >> + struct user *role =3D = user_by_id(priv->object_id); >> + if (role =3D=3D NULL || role->def->type !=3D = SC_ROLE) { >> + tnt_raise(ClientError, ER_NO_SUCH_ROLE, >> + role ? role->def->name : >> + int2str(priv->object_id)); >> + } >> + /* >> + * Only the creator of the role can grant or = revoke it. >> + * Everyone can grant 'PUBLIC' role. >> + */ >> + if (role->def->owner !=3D grantor->def->uid && >> + grantor->def->uid !=3D ADMIN && >> + (role->def->uid !=3D PUBLIC || priv->access = !=3D PRIV_X)) { >> + tnt_raise(AccessDeniedError, >> + priv_name(priv_type), >> + schema_object_name(SC_ROLE), = name, >> + grantor->def->name); >> + } >> + /* Not necessary to do during revoke, but who = cares. */ >> + role_check(grantee, role); >> + } else if (grantor->def->uid !=3D ADMIN) { >> + /* only admin may grant privileges on an entire = entity. */ >=20 >> tnt_raise(AccessDeniedError, >> priv_name(priv_type), >> schema_object_name(SC_ROLE), name, >> grantor->def->name); >> } >> - /* Not necessary to do during revoke, but who cares. */ >> - role_check(grantee, role); >=20 > Why no 'break;'? >=20 > Instead of changing indent level you could have added a break. Revorked this part, it is not so bulky anymore, see the new diff below. >> + case SC_USER: >> + { >> + /* >> + * user ID 0 is shared between user 'guest' and granting >> + * privileges upon whole entity user. This is not a = problem, >> + * since we don't want to grant privileges on any system = user, >> + * including 'guest'. >> + */ >> + if(priv->object_id =3D=3D 0) { >> + access =3D entity_access.user; >> + break; >> + } >=20 > Hm, does it mean that now it's not possible to grant some > privileges to 'guest'? Looks like we should be using a different > identifier for entity-level grants then? > Are you testing this at all? (I found no test for this). Revorked this part as you requested. src/box/alter.cc | 43 = ++++++++++++++++++++++++++++++++++++++++--- src/box/lua/schema.lua | 35 +++++++++++++++++++++++++---------- src/box/schema.h | 8 ++++++++ src/box/user.cc | 22 +++++++++++++++++++++- test/box/access.result | 20 ++++++++++---------- test/box/access.test.lua | 15 +++++---------- test/box/access_misc.result | 2 +- test/box/role.result | 25 +++++++++++++++++++++++-- test/box/role.test.lua | 12 ++++++++++-- 9 files changed, 143 insertions(+), 39 deletions(-) diff --git a/src/box/alter.cc b/src/box/alter.cc index 3007a131d..d4545f31c 100644 --- a/src/box/alter.cc +++ b/src/box/alter.cc @@ -2170,7 +2170,7 @@ on_replace_dd_user(struct trigger * /* trigger */, = void *event) struct user *old_user =3D user_by_id(uid); if (new_tuple !=3D NULL && old_user =3D=3D NULL) { /* INSERT */ struct user_def *user =3D = user_def_new_from_tuple(new_tuple); - access_check_ddl(user->name, user->owner, SC_USER, = PRIV_C, true); + access_check_ddl(user->name, user->owner, user->type, = PRIV_C, true); auto def_guard =3D make_scoped_guard([=3D] { free(user); = }); (void) user_cache_replace(user); def_guard.is_active =3D false; @@ -2179,7 +2179,7 @@ on_replace_dd_user(struct trigger * /* trigger */, = void *event) txn_on_rollback(txn, on_rollback); } else if (new_tuple =3D=3D NULL) { /* DELETE */ access_check_ddl(old_user->def->name, = old_user->def->owner, - SC_USER, PRIV_D, true); + old_user->def->type, PRIV_D, true); /* Can't drop guest or super user */ if (uid <=3D (uint32_t) BOX_SYSTEM_USER_ID_MAX || uid =3D=3D= SUPER) { tnt_raise(ClientError, ER_DROP_USER, @@ -2645,6 +2645,38 @@ priv_def_check(struct priv_def *priv, enum = priv_type priv_type) } /* Not necessary to do during revoke, but who cares. */ role_check(grantee, role); + break; + } + case SC_USER: + { + struct user *user =3D NULL; + user =3D user_by_id(priv->object_id); + if (user =3D=3D NULL || user->def->type !=3D SC_USER) { + tnt_raise(ClientError, ER_NO_SUCH_USER, + user ? user->def->name : + int2str(priv->object_id)); + } + if (user->def->owner !=3D grantor->def->uid && + grantor->def->uid !=3D ADMIN) { + tnt_raise(AccessDeniedError, + priv_name(priv_type), + schema_object_name(SC_USER), name, + grantor->def->name); + } + break; + } + case SC_ENTITY_SPACE: + case SC_ENTITY_FUNCTION: + case SC_ENTITY_SEQUENCE: + case SC_ENTITY_ROLE: + case SC_ENTITY_USER: + { + /* Only amdin may grant privileges on an entire entity. = */ + if (grantor->def->uid !=3D ADMIN) { + tnt_raise(AccessDeniedError, = priv_name(priv_type), + schema_object_name(priv->object_type), = name, + grantor->def->name); + } } default: break; @@ -2665,7 +2697,12 @@ grant_or_revoke(struct priv_def *priv) struct user *grantee =3D user_by_id(priv->grantee_id); if (grantee =3D=3D NULL) return; - if (priv->object_type =3D=3D SC_ROLE) { + /* + * Grant a role to a user only when privilege type is 'execute' + * and the role is specified. + */ + if (priv->object_type =3D=3D SC_ROLE && !(priv->access & = ~PRIV_X) && + priv->object_id !=3D 0) { struct user *role =3D user_by_id(priv->object_id); if (role =3D=3D NULL || role->def->type !=3D SC_ROLE) return; diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua index eb984b1de..28f68cbda 100644 --- a/src/box/lua/schema.lua +++ b/src/box/lua/schema.lua @@ -1746,6 +1746,9 @@ local priv_object_combo =3D { ["role"] =3D bit.bor(box.priv.X, box.priv.U, box.priv.D), ["all roles"] =3D bit.bor(box.priv.C, box.priv.D), + ["user"] =3D bit.bor(box.priv.A, box.priv.D), + ["all users"] =3D bit.bor(box.priv.C, box.priv.A, + box.priv.D), } =20 -- @@ -1839,6 +1842,12 @@ local function object_resolve(object_type, = object_name) end return 0 end + if object_type =3D=3D 'all users' then + if object_name ~=3D nil and object_name ~=3D 0 then + box.error(box.error.ILLEGAL_PARAMS, "no object name = allowed") + end + return 0 + end if object_type =3D=3D 'space' then if object_name =3D=3D nil then return nil @@ -1876,22 +1885,27 @@ local function object_resolve(object_type, = object_name) end return seq end - if object_type =3D=3D 'role' then - if object_name =3D=3D nil then + if object_type =3D=3D 'role' or object_type =3D=3D 'user' then + if object_name =3D=3D nil then return nil end local _vuser =3D box.space[box.schema.VUSER_ID] - local role + local role_or_user + if object_name =3D=3D nil or object_name =3D=3D 0 then + return 0 + end if type(object_name) =3D=3D 'string' then - role =3D _vuser.index.name:get{object_name} + role_or_user =3D _vuser.index.name:get{object_name} else - role =3D _vuser:get{object_name} + role_or_user =3D _vuser:get{object_name} end - if role and role[4] =3D=3D 'role' then - return role[1] - else + if role_or_user and role_or_user[4] =3D=3D object_type then + return role_or_user[1] + elseif object_type =3D=3D 'role' then box.error(box.error.NO_SUCH_ROLE, object_name) - end + else + box.error(box.error.NO_SUCH_USER, object_name) + end end =20 box.error(box.error.UNKNOWN_SCHEMA_OBJECT, object_type) @@ -2146,7 +2160,8 @@ local function grant(uid, name, privilege, = object_type, if privilege_hex ~=3D old_privilege then _priv:replace{options.grantor, uid, object_type, oid, = privilege_hex} elseif not options.if_not_exists then - if object_type =3D=3D 'role' then + if object_type =3D=3D 'role' and object_name ~=3D nil and + object_name ~=3D 0 and privilege =3D=3D 'execute' then box.error(box.error.ROLE_GRANTED, name, object_name) else box.error(box.error.PRIV_GRANTED, name, privilege, diff --git a/src/box/schema.h b/src/box/schema.h index f1735ff34..c343650de 100644 --- a/src/box/schema.h +++ b/src/box/schema.h @@ -240,6 +240,8 @@ struct on_access_denied_ctx { struct entity_access { struct access space[BOX_USER_MAX]; struct access function[BOX_USER_MAX]; + struct access user[BOX_USER_MAX]; + struct access role[BOX_USER_MAX]; struct access sequence[BOX_USER_MAX]; }; =20 @@ -257,6 +259,12 @@ entity_access_get(enum schema_object_type type) case SC_FUNCTION: case SC_ENTITY_FUNCTION: return entity_access.function; + case SC_USER: + case SC_ENTITY_USER: + return entity_access.user; + case SC_ROLE: + case SC_ENTITY_ROLE: + return entity_access.role; case SC_SEQUENCE: case SC_ENTITY_SEQUENCE: return entity_access.sequence; diff --git a/src/box/user.cc b/src/box/user.cc index eec785652..b4fb65a59 100644 --- a/src/box/user.cc +++ b/src/box/user.cc @@ -217,6 +217,16 @@ access_find(struct priv_def *priv) access =3D entity_access.function; break; } + case SC_ENTITY_USER: + { + access =3D entity_access.user; + break; + } + case SC_ENTITY_ROLE: + { + access =3D entity_access.role; + break; + } case SC_ENTITY_SEQUENCE: { access =3D entity_access.sequence; @@ -236,6 +246,16 @@ access_find(struct priv_def *priv) access =3D func->access; break; } + case SC_USER: + { + /* No grants on a single object user yet. */ + break; + } + case SC_ROLE: + { + /* No grants on a single object role yet. */ + break; + } case SC_SEQUENCE: { struct sequence *seq =3D = sequence_by_id(priv->object_id); @@ -318,7 +338,7 @@ user_reload_privs(struct user *user) * Skip role grants, we're only * interested in real objects. */ - if (priv.object_type !=3D SC_ROLE) + if (priv.object_type !=3D SC_ROLE || = !(priv.access & PRIV_X)) user_grant_priv(user, &priv); } } diff --git a/test/box/access.result b/test/box/access.result index 30fcb6455..7acd6fa43 100644 --- a/test/box/access.result +++ b/test/box/access.result @@ -878,8 +878,6 @@ box.schema.user.create('test') box.schema.user.grant('test', 'read', 'space', '_collation') --- ... ---box.schema.user.grant('test', 'write', 'space', '_collation') --- FIXME: granting create on 'collation' only doesn't work box.schema.user.grant('test', 'create', 'universe') --- ... @@ -1427,16 +1425,18 @@ box.session.su("admin") box.schema.user.grant('tester', 'write', 'universe') --- ... --- no entity user currently, so have to grant create --- on universe in order to create a user. -box.schema.user.grant('tester', 'create', 'universe') +box.schema.user.grant('tester', 'create', 'user') +--- +... +box.schema.user.grant('tester', 'create', 'space') +--- +... +box.schema.user.grant('tester', 'create', 'function') +--- +... +box.schema.user.grant('tester', 'create' , 'sequence') --- ... --- this should work instead: ---box.schema.user.grant('tester', 'create', 'user') ---box.schema.user.grant('tester', 'create', 'space') ---box.schema.user.grant('tester', 'create', 'function') ---box.schema.user.grant('tester', 'create' , 'sequence') box.schema.user.grant('tester', 'read', 'space', '_sequence') --- ... diff --git a/test/box/access.test.lua b/test/box/access.test.lua index 9ae0e1114..9b7510e64 100644 --- a/test/box/access.test.lua +++ b/test/box/access.test.lua @@ -341,8 +341,7 @@ c:close() session =3D box.session box.schema.user.create('test') box.schema.user.grant('test', 'read', 'space', '_collation') ---box.schema.user.grant('test', 'write', 'space', '_collation') --- FIXME: granting create on 'collation' only doesn't work + box.schema.user.grant('test', 'create', 'universe') session.su('test') box.internal.collation.create('test', 'ICU', 'ru_RU') @@ -538,14 +537,10 @@ box.session.su("admin") -- tables from ddl -- box.schema.user.grant('tester', 'write', 'universe') --- no entity user currently, so have to grant create --- on universe in order to create a user. -box.schema.user.grant('tester', 'create', 'universe') --- this should work instead: ---box.schema.user.grant('tester', 'create', 'user') ---box.schema.user.grant('tester', 'create', 'space') ---box.schema.user.grant('tester', 'create', 'function') ---box.schema.user.grant('tester', 'create' , 'sequence') +box.schema.user.grant('tester', 'create', 'user') +box.schema.user.grant('tester', 'create', 'space') +box.schema.user.grant('tester', 'create', 'function') +box.schema.user.grant('tester', 'create' , 'sequence') box.schema.user.grant('tester', 'read', 'space', '_sequence') box.session.su("tester") -- successful create diff --git a/test/box/access_misc.result b/test/box/access_misc.result index 2d87fa2d5..b7f0b7031 100644 --- a/test/box/access_misc.result +++ b/test/box/access_misc.result @@ -361,7 +361,7 @@ testuser_uid =3D session.uid() ... _ =3D box.space._user:delete(2) --- -- error: Drop access to user 'public' is denied for user 'testuser' +- error: Drop access to role 'public' is denied for user 'testuser' ... box.space._user:select(1) --- diff --git a/test/box/role.result b/test/box/role.result index 806cea90b..243c7bc6c 100644 --- a/test/box/role.result +++ b/test/box/role.result @@ -214,7 +214,22 @@ box.schema.role.drop('test') box.schema.user.grant('grantee', 'liaison') --- ... -box.schema.user.grant('test', 'read,write,create', 'universe') +box.schema.user.grant('test', 'read,write', 'space', '_priv') +--- +... +box.schema.user.grant('test', 'write', 'space', '_schema') +--- +... +box.schema.user.grant('test', 'create', 'space') +--- +... +box.schema.user.grant('test', 'read,write', 'space', '_space') +--- +... +box.schema.user.grant('test', 'write', 'space', '_index') +--- +... +box.schema.user.grant('test', 'read', 'space', '_user') --- ... box.session.su('test') @@ -635,7 +650,13 @@ box.schema.user.create('user') box.schema.user.create('grantee') --- ... -box.schema.user.grant('user', 'read,write,execute,create', 'universe') +box.schema.user.grant('user', 'read,write', 'space', '_user') +--- +... +box.schema.user.grant('user', 'read,write', 'space', '_priv') +--- +... +box.schema.user.grant('user', 'create', 'role') --- ... box.session.su('user') diff --git a/test/box/role.test.lua b/test/box/role.test.lua index e97339f49..9845f4c4c 100644 --- a/test/box/role.test.lua +++ b/test/box/role.test.lua @@ -69,7 +69,13 @@ box.schema.role.revoke('test', 'liaison') box.schema.role.drop('test') =20 box.schema.user.grant('grantee', 'liaison') -box.schema.user.grant('test', 'read,write,create', 'universe') + +box.schema.user.grant('test', 'read,write', 'space', '_priv') +box.schema.user.grant('test', 'write', 'space', '_schema') +box.schema.user.grant('test', 'create', 'space') +box.schema.user.grant('test', 'read,write', 'space', '_space') +box.schema.user.grant('test', 'write', 'space', '_index') +box.schema.user.grant('test', 'read', 'space', '_user') box.session.su('test') s =3D box.schema.space.create('test') _ =3D s:create_index('i1') @@ -248,7 +254,9 @@ box.schema.role.drop("role10") box.schema.user.create('user') box.schema.user.create('grantee') =20 -box.schema.user.grant('user', 'read,write,execute,create', 'universe') +box.schema.user.grant('user', 'read,write', 'space', '_user') +box.schema.user.grant('user', 'read,write', 'space', '_priv') +box.schema.user.grant('user', 'create', 'role') box.session.su('user') box.schema.role.create('role') box.session.su('admin') --=20 2.15.2 (Apple Git-101.1)