Tarantool development patches archive
 help / color / mirror / Atom feed
From: Konstantin Osipov <kostja@tarantool.org>
To: tarantool-patches@freelists.org
Cc: Serge Petrenko <sergepetrenko@tarantool.org>
Subject: [tarantool-patches] Re: [PATCH] Make access_check_ddl check for entity privileges.
Date: Wed, 11 Jul 2018 21:33:58 +0300	[thread overview]
Message-ID: <20180711183358.GA31667@chai> (raw)
In-Reply-To: <20180711164036.33604-1-sergepetrenko@tarantool.org>

* Serge Petrenko <sergepetrenko@tarantool.org> [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

  reply	other threads:[~2018-07-11 18:34 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-07-11 16:40 [tarantool-patches] " Serge Petrenko
2018-07-11 18:33 ` Konstantin Osipov [this message]
2018-07-12  8:52   ` [tarantool-patches] " Sergey Petrenko
2018-07-18  6:07     ` Konstantin Osipov
     [not found] <c734dd77-57dd-b0a3-af26-bf38937b1725@tarantool.org>
2018-07-19  7:48 ` Sergey Petrenko

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180711183358.GA31667@chai \
    --to=kostja@tarantool.org \
    --cc=sergepetrenko@tarantool.org \
    --cc=tarantool-patches@freelists.org \
    --subject='[tarantool-patches] Re: [PATCH] Make access_check_ddl check for entity privileges.' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox