[tarantool-patches] Re: [PATCH v2 3/4] Add single object privilege checks to access_check_ddl.
Konstantin Osipov
kostja at tarantool.org
Thu Jul 26 23:37:31 MSK 2018
* Serge Petrenko <sergepetrenko at tarantool.org> [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
More information about the Tarantool-patches
mailing list