[tarantool-patches] Re: [PATCH] Make access_check_ddl check for entity privileges.

[Konstantin Osipov]

* Serge Petrenko <sergepetrenko at 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