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

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

[Serge Petrenko]

Function access_check_ddl checked only for universal access, thus
granting entity or singe object access to a user would have no effect in
scope of this function.
Fix this by adding entity access checks.

Also attaching an existing sequence to a space checked for
create privilege on both space and sequence
(instead of read + write on sequence). Fixed it and changed the tests
accordingly.

Closes #3516
---
https://github.com/tarantool/tarantool/issues/3516
https://github.com/tarantool/tarantool/tree/sergepetrenko/gh-3516-entity-access-checks

Changes in v3
  - creating / altering / dropping an index
    checks for create / alter / drop privilege
    on space respectively.
    Adding correct index privileges is moved to
    a separate patch.
Changes in v2
  - rebased to the latest 1.10

 src/box/alter.cc           |  26 ++++++++--
 test/box/access.result     |  29 +++++++++--
 test/box/access.test.lua   |  21 ++++++--
 test/box/sequence.result   | 125 ++++++++++++++++++++++++++++++++++++++++-----
 test/box/sequence.test.lua |  57 ++++++++++++++++-----
 5 files changed, 220 insertions(+), 38 deletions(-)

diff --git a/src/box/alter.cc b/src/box/alter.cc
index b74369321..3007a131d 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -85,6 +85,13 @@ access_check_ddl(const char *name, uint32_t owner_uid,
 	user_access_t access = ((PRIV_U | (user_access_t) priv_type) &
 				~has_access);
 	bool is_owner = owner_uid == cr->uid || cr->uid == ADMIN;
+	if (access == 0)
+		return; /* Access granted. */
+	/* Check for specific entity access. */
+	struct access *object = entity_access_get(type);
+	if (object) {
+		access &= ~object[cr->auth_token].effective;
+	}
 	/*
 	 * Only the owner of the object or someone who has
 	 * specific DDL privilege on the object can execute
@@ -94,7 +101,7 @@ access_check_ddl(const char *name, uint32_t owner_uid,
 	 * the owner of the object, but this should be ignored --
 	 * CREATE privilege is required.
 	 */
-	if (access == 0 || (is_owner && !(access & (PRIV_U|PRIV_C))))
+	if (access == 0 || (is_owner && !(access & (PRIV_U | PRIV_C))))
 		return; /* Access granted. */
 
 	/* Create a meaningful error message. */
@@ -3032,7 +3039,7 @@ on_replace_dd_sequence(struct trigger * /* trigger */, void *event)
 						      ER_CREATE_SEQUENCE);
 		assert(sequence_by_id(new_def->id) == NULL);
 		access_check_ddl(new_def->name, new_def->uid, SC_SEQUENCE,
-			PRIV_C, false);
+				 PRIV_C, false);
 		sequence_cache_replace(new_def);
 		alter->new_def = new_def;
 	} else if (old_tuple != NULL && new_tuple == NULL) {	/* DELETE */
@@ -3137,8 +3144,19 @@ on_replace_dd_space_sequence(struct trigger * /* trigger */, void *event)
 		priv_type = PRIV_A;
 
 	/* Check we have the correct access type on the sequence.  * */
-	access_check_ddl(seq->def->name, seq->def->uid, SC_SEQUENCE, priv_type,
-			 false);
+	if (is_generated || !stmt->new_tuple) {
+		access_check_ddl(seq->def->name, seq->def->uid, SC_SEQUENCE,
+				 priv_type, false);
+	} else {
+		/*
+		 * In case user wants to attach an existing sequence,
+		 * 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);
+	}
 	/** Check we have alter access on space. */
 	access_check_ddl(space->def->name, space->def->uid, SC_SPACE, PRIV_A,
 			 false);
diff --git a/test/box/access.result b/test/box/access.result
index a1f3e996a..f4669a4a3 100644
--- a/test/box/access.result
+++ b/test/box/access.result
@@ -875,7 +875,12 @@ session = box.session
 box.schema.user.create('test')
 ---
 ...
-box.schema.user.grant('test', 'read,write,create', 'universe')
+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')
@@ -1389,7 +1394,10 @@ box.schema.func.create('test_func')
 box.session.su("admin")
 ---
 ...
-box.schema.user.grant("tester", "read", "universe")
+box.schema.user.grant("tester", "read", "space", "_user")
+---
+...
+box.schema.user.grant("tester", "read", "space", "_func")
 ---
 ...
 -- failed create
@@ -1416,7 +1424,20 @@ box.session.su("admin")
 -- explicitly since we still use process_rw to write to system
 -- tables from ddl
 --
-box.schema.user.grant("tester", "create,write", "universe")
+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', 'read', 'space', '_sequence')
 ---
 ...
 box.session.su("tester")
@@ -1824,7 +1845,7 @@ _  = box.schema.sequence.create('test_sequence')
 box.session.su('admin')
 ---
 ...
-box.schema.user.grant('tester', 'create', 'universe')
+box.schema.user.grant('tester', 'create', 'sequence')
 ---
 ...
 box.session.su('tester')
diff --git a/test/box/access.test.lua b/test/box/access.test.lua
index fb8f744e8..9ae0e1114 100644
--- a/test/box/access.test.lua
+++ b/test/box/access.test.lua
@@ -340,7 +340,10 @@ c:close()
 
 session = box.session
 box.schema.user.create('test')
-box.schema.user.grant('test', 'read,write,create', 'universe')
+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')
 session.su('admin')
@@ -520,7 +523,8 @@ box.schema.space.create("test_space")
 box.schema.user.create('test_user')
 box.schema.func.create('test_func')
 box.session.su("admin")
-box.schema.user.grant("tester", "read", "universe")
+box.schema.user.grant("tester", "read", "space", "_user")
+box.schema.user.grant("tester", "read", "space", "_func")
 -- failed create
 box.session.su("tester")
 box.schema.space.create("test_space")
@@ -533,7 +537,16 @@ box.session.su("admin")
 -- explicitly since we still use process_rw to write to system
 -- tables from ddl
 --
-box.schema.user.grant("tester", "create,write", "universe")
+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', 'read', 'space', '_sequence')
 box.session.su("tester")
 -- successful create
 s1 = box.schema.space.create("test_space")
@@ -712,7 +725,7 @@ box.schema.user.grant('tester', 'read,write', 'space', '_sequence')
 box.session.su('tester')
 _  = box.schema.sequence.create('test_sequence')
 box.session.su('admin')
-box.schema.user.grant('tester', 'create', 'universe')
+box.schema.user.grant('tester', 'create', 'sequence')
 box.session.su('tester')
 _ = box.schema.sequence.create('test_sequence')
 box.session.su('admin')
diff --git a/test/box/sequence.result b/test/box/sequence.result
index cbbd45080..a2a1a60ea 100644
--- a/test/box/sequence.result
+++ b/test/box/sequence.result
@@ -1471,8 +1471,17 @@ box.session.su('admin')
 sq:drop()
 ---
 ...
+box.schema.user.revoke('user', 'read,write', 'universe')
+---
+...
 -- A user can alter/use sequences that he owns.
-box.schema.user.grant('user', 'create', 'universe')
+box.schema.user.grant('user', 'create', 'sequence')
+---
+...
+box.schema.user.grant('user', 'write', 'space', '_sequence')
+---
+...
+box.schema.user.grant('user', 'write', 'space', '_sequence_data')
 ---
 ...
 box.session.su('user')
@@ -1493,7 +1502,13 @@ sq = box.schema.sequence.create('seq')
 box.session.su('admin')
 ---
 ...
-box.schema.user.revoke('user', 'read,write,create', 'universe')
+box.schema.user.revoke('user', 'create', 'sequence')
+---
+...
+box.schema.user.revoke('user', 'write', 'space', '_sequence')
+---
+...
+box.schema.user.revoke('user', 'write', 'space', '_sequence_data')
 ---
 ...
 box.session.su('user')
@@ -1515,7 +1530,8 @@ box.session.su('admin')
 sq:drop()
 ---
 ...
--- A sequence can be attached to a space only if the user owns both.
+-- A sequence can be attached to a space only if the user has
+-- create privilege on space and read/write on sequence.
 sq1 = box.schema.sequence.create('seq1')
 ---
 ...
@@ -1525,10 +1541,22 @@ s1 = box.schema.space.create('space1')
 _ = s1:create_index('pk')
 ---
 ...
-box.schema.user.grant('user', 'read,write', 'universe')
+box.schema.user.grant('user', 'write', 'space', '_sequence')
+---
+...
+box.schema.user.grant('user', 'write', 'space', '_sequence_data')
+---
+...
+box.schema.user.grant('user', 'write', 'space', '_schema')
+---
+...
+box.schema.user.grant('user', 'write', 'space', '_space')
 ---
 ...
-box.schema.user.grant('user', 'create', 'universe')
+box.schema.user.grant('user', 'create', 'space')
+---
+...
+box.schema.user.grant('user', 'create', 'sequence')
 ---
 ...
 box.session.su('user')
@@ -1540,17 +1568,50 @@ sq2 = box.schema.sequence.create('seq2')
 s2 = box.schema.space.create('space2')
 ---
 ...
--- fixme: no error on using another user's sequence
-_ = s2:create_index('pk', {sequence = 'seq1'})
+box.session.su('admin')
+---
+...
+box.schema.user.revoke('user', 'create', 'sequence')
+---
+...
+box.schema.user.revoke('user', 'write', 'space', '_sequence')
+---
+...
+box.schema.user.revoke('user', 'write', 'space', '_sequence_data')
 ---
 ...
+box.schema.user.revoke('user', 'write', 'space', '_schema')
+---
+...
+box.schema.user.revoke('user', 'write', 'space', '_space')
+---
+...
+box.schema.user.grant('user', 'write', 'space', '_index')
+---
+...
+box.schema.user.grant('user', 'write', 'space', '_space_sequence')
+---
+...
+box.schema.user.grant('user', 'read', 'space', '_index')
+---
+...
+box.schema.user.grant('user', 'read', 'space', '_space_sequence')
+---
+...
+box.session.su('user')
+---
+...
+_ = s2:create_index('pk', {sequence = 'seq1'}) -- error
+---
+- error: Read access to sequence 'seq1' is denied for user 'user'
+...
 s1.index.pk:alter({sequence = 'seq1'}) -- error
 ---
 - error: Alter access to space 'space1' is denied for user 'user'
 ...
 box.space._space_sequence:replace{s1.id, sq1.id, false} -- error
 ---
-- error: Alter access to space 'space1' is denied for user 'user'
+- error: Read access to sequence 'seq1' is denied for user 'user'
 ...
 box.space._space_sequence:replace{s1.id, sq2.id, false} -- error
 ---
@@ -1558,7 +1619,7 @@ box.space._space_sequence:replace{s1.id, sq2.id, false} -- error
 ...
 box.space._space_sequence:replace{s2.id, sq1.id, false} -- error
 ---
-- error: Alter access to sequence 'seq1' is denied for user 'user'
+- error: Read access to sequence 'seq1' is denied for user 'user'
 ...
 s2.index.pk:alter({sequence = 'seq2'}) -- ok
 ---
@@ -1569,10 +1630,22 @@ box.session.su('admin')
 -- If the user owns a sequence attached to a space,
 -- it can use it for auto increment, otherwise it
 -- needs privileges.
-box.schema.user.revoke('user', 'read,write', 'universe')
+box.schema.user.revoke('user', 'write', 'space', '_index')
+---
+...
+box.schema.user.revoke('user', 'write', 'space', '_space_sequence')
+---
+...
+box.schema.user.revoke('user', 'read', 'space', '_space')
+---
+...
+box.schema.user.revoke('user', 'read', 'space', '_sequence')
+---
+...
+box.schema.user.revoke('user', 'read', 'space', '_index')
 ---
 ...
-box.schema.user.revoke('user', 'create', 'universe')
+box.schema.user.revoke('user', 'read', 'space', '_space_sequence')
 ---
 ...
 box.session.su('user')
@@ -1680,7 +1753,16 @@ s:drop()
 ---
 ...
 -- When a user is dropped, all his sequences are dropped as well.
-box.schema.user.grant('user', 'read,write,create', 'universe')
+box.schema.user.grant('user', 'write', 'space', '_sequence')
+---
+...
+box.schema.user.grant('user', 'read', 'space', '_sequence')
+---
+...
+box.schema.user.grant('user', 'write', 'space', '_space_sequence')
+---
+...
+box.schema.user.grant('user', 'create', 'sequence')
 ---
 ...
 box.session.su('user')
@@ -1710,10 +1792,25 @@ box.schema.user.create('user1')
 box.schema.user.create('user2')
 ---
 ...
-box.schema.user.grant('user1', 'read,write,create', 'universe')
+box.schema.user.grant('user1', 'create', 'sequence')
+---
+...
+box.schema.user.grant('user1', 'write', 'space', '_sequence')
+---
+...
+box.schema.user.grant('user1', 'read', 'space', '_sequence')
+---
+...
+box.schema.user.grant('user1', 'read', 'space', '_user')
+---
+...
+box.schema.user.grant('user1', 'write', 'space', '_sequence_data')
+---
+...
+box.schema.user.grant('user1', 'write', 'space', '_priv')
 ---
 ...
-box.schema.user.grant('user2', 'read,write,create', 'universe')
+box.schema.user.grant('user2', 'read,write', 'universe')
 ---
 ...
 box.session.su('user1')
diff --git a/test/box/sequence.test.lua b/test/box/sequence.test.lua
index c119459b3..96297d6f2 100644
--- a/test/box/sequence.test.lua
+++ b/test/box/sequence.test.lua
@@ -488,16 +488,21 @@ sq:alter{step = 2} -- error
 sq:drop() -- error
 box.session.su('admin')
 sq:drop()
+box.schema.user.revoke('user', 'read,write', 'universe')
 
 -- A user can alter/use sequences that he owns.
-box.schema.user.grant('user', 'create', 'universe')
+box.schema.user.grant('user', 'create', 'sequence')
+box.schema.user.grant('user', 'write', 'space', '_sequence')
+box.schema.user.grant('user', 'write', 'space', '_sequence_data')
 box.session.su('user')
 sq = box.schema.sequence.create('seq')
 sq:alter{step = 2} -- ok
 sq:drop() -- ok
 sq = box.schema.sequence.create('seq')
 box.session.su('admin')
-box.schema.user.revoke('user', 'read,write,create', 'universe')
+box.schema.user.revoke('user', 'create', 'sequence')
+box.schema.user.revoke('user', 'write', 'space', '_sequence')
+box.schema.user.revoke('user', 'write', 'space', '_sequence_data')
 box.session.su('user')
 sq:set(100) -- ok - user owns the sequence
 sq:next() -- ok
@@ -505,17 +510,33 @@ sq:reset() -- ok
 box.session.su('admin')
 sq:drop()
 
--- A sequence can be attached to a space only if the user owns both.
+-- A sequence can be attached to a space only if the user has
+-- create privilege on space and read/write on sequence.
 sq1 = box.schema.sequence.create('seq1')
 s1 = box.schema.space.create('space1')
 _ = s1:create_index('pk')
-box.schema.user.grant('user', 'read,write', 'universe')
-box.schema.user.grant('user', 'create', 'universe')
+box.schema.user.grant('user', 'write', 'space', '_sequence')
+box.schema.user.grant('user', 'write', 'space', '_sequence_data')
+box.schema.user.grant('user', 'write', 'space', '_schema')
+box.schema.user.grant('user', 'write', 'space', '_space')
+box.schema.user.grant('user', 'create', 'space')
+box.schema.user.grant('user', 'create', 'sequence')
 box.session.su('user')
 sq2 = box.schema.sequence.create('seq2')
 s2 = box.schema.space.create('space2')
--- fixme: no error on using another user's sequence
-_ = s2:create_index('pk', {sequence = 'seq1'})
+
+box.session.su('admin')
+box.schema.user.revoke('user', 'create', 'sequence')
+box.schema.user.revoke('user', 'write', 'space', '_sequence')
+box.schema.user.revoke('user', 'write', 'space', '_sequence_data')
+box.schema.user.revoke('user', 'write', 'space', '_schema')
+box.schema.user.revoke('user', 'write', 'space', '_space')
+box.schema.user.grant('user', 'write', 'space', '_index')
+box.schema.user.grant('user', 'write', 'space', '_space_sequence')
+box.schema.user.grant('user', 'read', 'space', '_index')
+box.schema.user.grant('user', 'read', 'space', '_space_sequence')
+box.session.su('user')
+_ = s2:create_index('pk', {sequence = 'seq1'}) -- error
 s1.index.pk:alter({sequence = 'seq1'}) -- error
 box.space._space_sequence:replace{s1.id, sq1.id, false} -- error
 box.space._space_sequence:replace{s1.id, sq2.id, false} -- error
@@ -526,8 +547,12 @@ box.session.su('admin')
 -- If the user owns a sequence attached to a space,
 -- it can use it for auto increment, otherwise it
 -- needs privileges.
-box.schema.user.revoke('user', 'read,write', 'universe')
-box.schema.user.revoke('user', 'create', 'universe')
+box.schema.user.revoke('user', 'write', 'space', '_index')
+box.schema.user.revoke('user', 'write', 'space', '_space_sequence')
+box.schema.user.revoke('user', 'read', 'space', '_space')
+box.schema.user.revoke('user', 'read', 'space', '_sequence')
+box.schema.user.revoke('user', 'read', 'space', '_index')
+box.schema.user.revoke('user', 'read', 'space', '_space_sequence')
 box.session.su('user')
 s2:insert{nil, 1} -- ok: {1, 1}
 box.session.su('admin')
@@ -563,7 +588,10 @@ box.session.su('admin')
 s:drop()
 
 -- When a user is dropped, all his sequences are dropped as well.
-box.schema.user.grant('user', 'read,write,create', 'universe')
+box.schema.user.grant('user', 'write', 'space', '_sequence')
+box.schema.user.grant('user', 'read', 'space', '_sequence')
+box.schema.user.grant('user', 'write', 'space', '_space_sequence')
+box.schema.user.grant('user', 'create', 'sequence')
 box.session.su('user')
 _ = box.schema.sequence.create('test1')
 _ = box.schema.sequence.create('test2')
@@ -575,8 +603,13 @@ box.sequence
 -- to a sequence.
 box.schema.user.create('user1')
 box.schema.user.create('user2')
-box.schema.user.grant('user1', 'read,write,create', 'universe')
-box.schema.user.grant('user2', 'read,write,create', 'universe')
+box.schema.user.grant('user1', 'create', 'sequence')
+box.schema.user.grant('user1', 'write', 'space', '_sequence')
+box.schema.user.grant('user1', 'read', 'space', '_sequence')
+box.schema.user.grant('user1', 'read', 'space', '_user')
+box.schema.user.grant('user1', 'write', 'space', '_sequence_data')
+box.schema.user.grant('user1', 'write', 'space', '_priv')
+box.schema.user.grant('user2', 'read,write', 'universe')
 box.session.su('user1')
 sq = box.schema.sequence.create('test')
 box.session.su('user2')
-- 
2.15.2 (Apple Git-101.1)

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

[Konstantin Osipov]

* Serge Petrenko <sergepetrenko@tarantool.org> [18/07/19 23:28]:
> Function access_check_ddl checked only for universal access, thus
> granting entity or singe object access to a user would have no effect in
> scope of this function.
> Fix this by adding entity access checks.
> 
> Also attaching an existing sequence to a space checked for
> create privilege on both space and sequence
> (instead of read + write on sequence). Fixed it and changed the tests
> accordingly.
> 
> Closes #3516

Pushed.


-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov