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

  • From: Konstantin Osipov <kostja@xxxxxxxxxxxxx>
  • To: tarantool-patches@xxxxxxxxxxxxx
  • Date: Wed, 11 Jul 2018 21:33:58 +0300

* Serge Petrenko <sergepetrenko@xxxxxxxxxxxxx> [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

Other related posts:

  1. Home
  2. tarantool-patches
  3. Archive
  4. 07-2018