From 7e8d2652483b348fac57b8321ebcf596cd946eec Mon Sep 17 00:00:00 2001
From: Dmitry Rodionov <d.rodionov@picodata.io>
Date: Thu, 30 Nov 2023 19:35:25 +0300
Subject: [PATCH] feat: validate combination of object type and privilege in
PrivilegeDef
At the later stage I discovered that sbroad applies the same validation
rules during parsing of grant/revoke statements and even its own
Privilege enum that is a ~copy of our PrivilegeType. Unfortunately
at the moment there is no way to share code between picodata and sbroad
efficiently (now only tarantool-module is shared and it is not suitable
for such kind of things) so it still makes sense to have this in
picodata because this is the point where all APIs converge to the single
point (CaS). In the future all other ways of validation should be
removed. Aside from sbroad similar kind of validation is performed
independently on lua API side.
Note that in prior commit 45ba7392bb078afe6fdd4fcad4e25f86d8d6bbf2 we've
removed all privileges from role super. This patch removes privileges
from admin that do not match the model: namely all privileges on
universe except session and usage. With this patch it is no longer possible
to grant or revoke such privileges.
---
src/access_control.rs | 6 +-
src/schema.rs | 155 ++++++++++++++++++++++++++++++++--------
src/sql.rs | 9 ++-
src/storage.rs | 5 +-
test/conftest.py | 26 +++++++
test/int/test_acl.py | 39 +++++-----
test/int/test_basics.py | 36 ++++------
test/int/test_sql.py | 5 --
8 files changed, 198 insertions(+), 83 deletions(-)
diff --git a/src/access_control.rs b/src/access_control.rs
index 374f89b131..b009a680f1 100644
--- a/src/access_control.rs
+++ b/src/access_control.rs
@@ -532,7 +532,8 @@ mod tests {
grantee_id,
grantor_id.unwrap_or(session::uid().unwrap()),
0,
- );
+ )
+ .expect("must be valid");
access_check_op(
&Op::Acl(Acl::GrantPrivilege {
@@ -559,7 +560,8 @@ mod tests {
grantee_id,
session::uid().unwrap(),
0,
- );
+ )
+ .expect("must be valid");
access_check_op(
&Op::Acl(Acl::RevokePrivilege {
diff --git a/src/schema.rs b/src/schema.rs
index a841dabed7..7988c8aff5 100644
--- a/src/schema.rs
+++ b/src/schema.rs
@@ -1,6 +1,7 @@
use once_cell::sync::OnceCell;
use std::borrow::Cow;
use std::collections::{BTreeMap, HashSet};
+use std::fmt::Display;
use std::time::Duration;
use tarantool::auth::AuthDef;
@@ -379,14 +380,20 @@ tarantool::define_str_enum! {
Drop = "drop",
/// ALTER
Alter = "alter",
- /// This is never granted, but used internally.
- Grant = "grant",
- /// Never granted, but used internally.
- Revoke = "revoke",
}
}
/// Privilege definition.
+/// Note the differences between picodata privileges and vanilla tarantool ones.
+/// 1) Super role in picodata is a placeholder. It exists because picodata reuses user
+/// ids from vanilla and super occupies an id. So to avoid assigning some piciodata
+/// user Id of the vanilla role super we keep it as a placeholder. Aside from that
+/// vanilla super role has some privileges that cannot be represented in picodata,
+/// namely privileges on universe. This hole opens possibility for unwanted edge cases.
+/// 2) Only Session and Usage can be granted on Universe.
+/// Note that validation is not performed in Deserialize. We assume that for untrusted data
+/// the object is created via constructor. In other cases validation was already performed
+/// prior to serialization thus deserialization always creates a valid object.
#[derive(Clone, Debug, Serialize, Deserialize, PartialEq, Eq, Hash)]
pub struct PrivilegeDef {
privilege: PrivilegeType,
@@ -410,6 +417,22 @@ pub struct PrivilegeDef {
impl Encode for PrivilegeDef {}
+#[derive(Debug, thiserror::Error, PartialEq, Eq)]
+pub struct InvalidPrivilegeError {
+ object_type: SchemaObjectType,
+ unsupported: PrivilegeType,
+ expected_one_of: &'static [PrivilegeType],
+}
+
+impl Display for InvalidPrivilegeError {
+ fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+ f.write_fmt(format_args!(
+ "Unsupported {} privilege '{}', expected one of {:?}",
+ self.object_type, self.unsupported, self.expected_one_of,
+ ))
+ }
+}
+
impl PrivilegeDef {
pub fn new(
privilege: PrivilegeType,
@@ -418,15 +441,43 @@ impl PrivilegeDef {
grantee_id: UserId,
grantor_id: UserId,
schema_version: u64,
- ) -> PrivilegeDef {
- PrivilegeDef {
+ ) -> Result<PrivilegeDef, InvalidPrivilegeError> {
+ let privilege_def = PrivilegeDef {
privilege,
object_type,
object_id,
grantee_id,
grantor_id,
schema_version,
+ };
+
+ use PrivilegeType::*;
+
+ let valid_privileges: &[PrivilegeType] = match privilege_def.object_type {
+ SchemaObjectType::Table => match privilege_def.object_id() {
+ Some(_) => &[Read, Write, Alter, Drop],
+ None => &[Read, Write, Create, Alter, Drop],
+ },
+ SchemaObjectType::Role => match privilege_def.object_id() {
+ Some(_) => &[Execute, Drop],
+ None => &[Create, Drop],
+ },
+ SchemaObjectType::User => match privilege_def.object_id() {
+ Some(_) => &[Alter, Drop],
+ None => &[Create, Alter, Drop],
+ },
+ SchemaObjectType::Universe => &[Session, Usage],
+ };
+
+ if !valid_privileges.contains(&privilege) {
+ return Err(InvalidPrivilegeError {
+ object_type,
+ unsupported: privilege,
+ expected_one_of: valid_privileges,
+ });
}
+
+ Ok(privilege_def)
}
#[inline(always)]
@@ -503,15 +554,18 @@ impl PrivilegeDef {
let mut v = Vec::new();
// SQL: GRANT <'usage', 'session'> ON 'universe' TO 'guest'
- for privilege in [PrivilegeType::Usage, PrivilegeType::Session] {
- v.push(PrivilegeDef {
- grantor_id: ADMIN_ID,
- grantee_id: GUEST_ID,
- object_type: SchemaObjectType::Universe,
- object_id: UNIVERSE_ID,
- privilege,
- schema_version: 0,
- });
+ // SQL: GRANT <'usage', 'session'> ON 'universe' TO 'admin'
+ for user in [GUEST_ID, ADMIN_ID] {
+ for privilege in [PrivilegeType::Usage, PrivilegeType::Session] {
+ v.push(PrivilegeDef {
+ grantor_id: ADMIN_ID,
+ grantee_id: user,
+ object_type: SchemaObjectType::Universe,
+ object_id: UNIVERSE_ID,
+ privilege,
+ schema_version: 0,
+ });
+ }
}
// SQL: GRANT 'public' TO 'guest'
@@ -524,18 +578,6 @@ impl PrivilegeDef {
schema_version: 0,
});
- // SQL: GRANT 'all privileges' ON 'universe' TO 'admin'
- for privilege in PrivilegeType::VARIANTS {
- v.push(PrivilegeDef {
- grantor_id: ADMIN_ID,
- grantee_id: ADMIN_ID,
- object_type: SchemaObjectType::Universe,
- object_id: UNIVERSE_ID,
- privilege: *privilege,
- schema_version: 0,
- });
- }
-
v
})
}
@@ -1266,7 +1308,7 @@ mod tests {
#[cfg(test)]
mod test {
use super::*;
- use tarantool::tuple::ToTupleBuffer;
+ use tarantool::tuple::{Decode, ToTupleBuffer};
#[test]
#[rustfmt::skip]
@@ -1318,4 +1360,61 @@ mod test {
let format = PrivilegeDef::format();
crate::util::check_tuple_matches_format(tuple_data.as_ref(), &format, "PrivilegeDef::format");
}
+
+ #[track_caller]
+ fn check_object_privilege(
+ object_type: SchemaObjectType,
+ valid_privileges: &'static [PrivilegeType],
+ object_id: i64,
+ ) {
+ for privilege in PrivilegeType::VARIANTS {
+ let res = PrivilegeDef::new(*privilege, object_type, object_id, 1, 1, 1);
+
+ if valid_privileges.contains(privilege) {
+ assert!(res.is_ok())
+ } else {
+ assert_eq!(
+ res.unwrap_err(),
+ InvalidPrivilegeError {
+ object_type,
+ unsupported: *privilege,
+ expected_one_of: valid_privileges,
+ }
+ )
+ }
+ }
+ }
+
+ #[test]
+ fn privilege_def_validation() {
+ use PrivilegeType::*;
+
+ // table
+ let valid = &[Read, Write, Create, Alter, Drop];
+ check_object_privilege(SchemaObjectType::Table, valid, -1);
+
+ // particular table
+ let valid = &[Read, Write, Alter, Drop];
+ check_object_privilege(SchemaObjectType::Table, valid, 42);
+
+ // user
+ let valid = &[Create, Alter, Drop];
+ check_object_privilege(SchemaObjectType::User, valid, -1);
+
+ // particular user
+ let valid = &[Alter, Drop];
+ check_object_privilege(SchemaObjectType::User, valid, 42);
+
+ // role
+ let valid = &[Create, Drop];
+ check_object_privilege(SchemaObjectType::Role, valid, -1);
+
+ // particular role
+ let valid = &[Execute, Drop];
+ check_object_privilege(SchemaObjectType::Role, valid, 42);
+
+ // universe
+ let valid = &[Session, Usage];
+ check_object_privilege(SchemaObjectType::Universe, valid, 0);
+ }
}
diff --git a/src/sql.rs b/src/sql.rs
index 407152b392..764136a8a1 100644
--- a/src/sql.rs
+++ b/src/sql.rs
@@ -800,7 +800,8 @@ fn reenterable_schema_change_request(
grantee_id,
grantor_id,
schema_version,
- );
+ )
+ .map_err(Error::other)?;
match alter_option_param {
AlterOptionParam::ChangePassword(auth) => {
@@ -912,7 +913,8 @@ fn reenterable_schema_change_request(
grantee_id,
grantor_id,
schema_version,
- ),
+ )
+ .map_err(Error::other)?,
})
}
Params::RevokePrivilege(revoke_type, grantee_name) => {
@@ -939,7 +941,8 @@ fn reenterable_schema_change_request(
grantee_id,
grantor_id,
schema_version,
- ),
+ )
+ .map_err(Error::other)?,
})
}
};
diff --git a/src/storage.rs b/src/storage.rs
index fb89dbade1..823d43fca2 100644
--- a/src/storage.rs
+++ b/src/storage.rs
@@ -2477,6 +2477,7 @@ impl Privileges {
/// Remove any privilege definitions assigning the given role.
#[inline]
pub fn delete_all_by_granted_role(&self, role_id: u32) -> Result<()> {
+ // here we failed to deserialize the privilege that has alter on universe
for priv_def in self.iter()? {
if priv_def.privilege() == PrivilegeType::Execute
&& priv_def.object_type() == SchemaObjectType::Role
@@ -2730,9 +2731,7 @@ impl SchemaDef for PrivilegeDef {
#[inline(always)]
fn schema_version(&self) -> u64 {
- // Note: this doesnt lead to recursion because of the
- // method resolution order: https://doc.rust-lang.org/reference/expressions/method-call-expr.html
- self.schema_version()
+ PrivilegeDef::schema_version(self)
}
#[inline(always)]
diff --git a/test/conftest.py b/test/conftest.py
index 6fd0c64382..0402a2f726 100644
--- a/test/conftest.py
+++ b/test/conftest.py
@@ -1354,6 +1354,32 @@ class Cluster:
eprint(f"CaS:\n {predicate=}\n {dml=}")
return instance.call("pico.cas", dml, predicate, user=user, password=password)
+ def masters(self) -> List[Instance]:
+ ret = []
+ for instance in self.instances:
+ if not instance.eval("return box.info.ro"):
+ ret.append(instance)
+
+ return ret
+
+ def grant_box_privilege(
+ self, user, privilege: str, object_type: str, object_name: Optional[str] = None
+ ):
+ """
+ Sometimes in our tests we go beyond picodata privilege model and need
+ to grant priveleges on something that is not part of the picodata access control model.
+ For example execute access on universe mainly needed to invoke functions.
+ """
+ for instance in self.masters():
+ instance.eval(
+ """
+ box.session.su("admin")
+ user, privilege, object_type, object_name = ...
+ return box.schema.user.grant(user, privilege, object_type, object_name)
+ """,
+ [user, privilege, object_type, object_name],
+ )
+
@dataclass
class PortalStorage:
diff --git a/test/int/test_acl.py b/test/int/test_acl.py
index 3c80452266..4279464498 100644
--- a/test/int/test_acl.py
+++ b/test/int/test_acl.py
@@ -202,9 +202,9 @@ def test_acl_lua_api(cluster: Cluster):
i1.call(
f"pico.{f}",
"User is not found",
- "execute",
- "universe",
- None,
+ "read",
+ "table",
+ "_pico_property",
)
# No such privilege -> error.
@@ -219,7 +219,7 @@ def test_acl_lua_api(cluster: Cluster):
ReturnError,
match=rf"unsupported privilege 'read,write', see pico.help\('{f}'\) for details",
):
- i1.call(f"pico.{f}", "Dave", "read,write", "universe", None)
+ i1.call(f"pico.{f}", "Dave", "read,write", "role", None)
# No object_type -> error.
with pytest.raises(ReturnError, match="object_type should be a string"):
@@ -463,9 +463,7 @@ def test_acl_basic(cluster: Cluster):
# Grant some privileges.
# Doing anything via remote function execution requires execute access
# to the "universe"
- index = i1.grant_privilege(user, "execute", "universe")
- cluster.raft_wait_index(index)
- v += 1
+ cluster.grant_box_privilege(user, "execute", "universe")
index = i1.grant_privilege(user, "read", "table", "money")
cluster.raft_wait_index(index)
@@ -592,8 +590,7 @@ def test_acl_roles_basic(cluster: Cluster):
# Doing anything via remote function execution requires execute access
# to the "universe"
- index = i1.grant_privilege(user, "execute", "universe", None)
- cluster.raft_wait_index(index)
+ cluster.grant_box_privilege(user, "execute", "universe", None)
# Try reading from table on behalf of the user.
for i in cluster.instances:
@@ -757,8 +754,8 @@ def test_acl_from_snapshot(cluster: Cluster):
("read", "space", "_pico_table"),
}
- with pytest.raises(TarantoolError, match="Role 'Executor' is not found"):
- i.call("box.schema.role.info", "Executor")
+ with pytest.raises(TarantoolError, match="Role 'Writer' is not found"):
+ i.call("box.schema.role.info", "Writer")
#
# These will be catching up by snapshot.
@@ -791,13 +788,13 @@ def test_acl_from_snapshot(cluster: Cluster):
)
cluster.raft_wait_index(index)
- index = i1.call("pico.create_role", "Executor")
+ index = i1.call("pico.create_role", "Writer")
cluster.raft_wait_index(index)
- index = i1.grant_privilege("Executor", "execute", "universe", None)
+ index = i1.grant_privilege("Writer", "write", "table", None)
cluster.raft_wait_index(index)
- index = i1.grant_privilege("Blam", "execute", "role", "Executor")
+ index = i1.grant_privilege("Blam", "execute", "role", "Writer")
cluster.raft_wait_index(index)
# Compact log to trigger snapshot generation.
@@ -822,7 +819,7 @@ def test_acl_from_snapshot(cluster: Cluster):
assert to_set_of_tuples(i.call("box.schema.user.info", "Blam")) == {
("execute", "role", "public"),
- ("execute", "role", "Executor"),
+ ("execute", "role", "Writer"),
("session,usage", "universe", ""),
("alter", "user", "Blam"),
}
@@ -831,8 +828,8 @@ def test_acl_from_snapshot(cluster: Cluster):
("read", "space", "_pico_instance"),
}
- assert to_set_of_tuples(i.call("box.schema.role.info", "Executor")) == {
- ("execute", "universe", ""),
+ assert to_set_of_tuples(i.call("box.schema.role.info", "Writer")) == {
+ ("write", "space", ""),
}
@@ -886,16 +883,18 @@ def test_builtin_users_and_roles(cluster: Cluster):
"_pico_property",
)
+ index = i1.call("pico.create_user", "Dave", VALID_PASSWORD)
+ i1.raft_wait_index(index)
+
# granting already granted privilege does not raise an error
index = i1.call("pico.raft_get_index")
new_index = i1.grant_privilege(
- "admin",
- "write",
+ "Dave",
+ "session",
"universe",
)
assert index == new_index
- index = i1.call("pico.create_user", "Dave", VALID_PASSWORD)
new_index = i1.grant_privilege(
"Dave",
"execute",
diff --git a/test/int/test_basics.py b/test/int/test_basics.py
index 5516edd052..e3011a8c5c 100644
--- a/test/int/test_basics.py
+++ b/test/int/test_basics.py
@@ -257,27 +257,19 @@ def test_raft_log(instance: Instance):
| 15 | 1 |1.0.15|Insert({_pico_role}, [31,"super",0,1])|
| 16 | 1 |1.0.16|Insert({_pico_privilege}, ["usage","universe",0,0,1,0])|
| 17 | 1 |1.0.17|Insert({_pico_privilege}, ["session","universe",0,0,1,0])|
-| 18 | 1 |1.0.18|Insert({_pico_privilege}, ["execute","role",2,0,1,0])|
-| 19 | 1 |1.0.19|Insert({_pico_privilege}, ["read","universe",0,1,1,0])|
-| 20 | 1 |1.0.20|Insert({_pico_privilege}, ["write","universe",0,1,1,0])|
-| 21 | 1 |1.0.21|Insert({_pico_privilege}, ["execute","universe",0,1,1,0])|
-| 22 | 1 |1.0.22|Insert({_pico_privilege}, ["session","universe",0,1,1,0])|
-| 23 | 1 |1.0.23|Insert({_pico_privilege}, ["usage","universe",0,1,1,0])|
-| 24 | 1 |1.0.24|Insert({_pico_privilege}, ["create","universe",0,1,1,0])|
-| 25 | 1 |1.0.25|Insert({_pico_privilege}, ["drop","universe",0,1,1,0])|
-| 26 | 1 |1.0.26|Insert({_pico_privilege}, ["alter","universe",0,1,1,0])|
-| 27 | 1 |1.0.27|Insert({_pico_privilege}, ["grant","universe",0,1,1,0])|
-| 28 | 1 |1.0.28|Insert({_pico_privilege}, ["revoke","universe",0,1,1,0])|
-| 29 | 1 | |AddNode(1)|
-| 30 | 2 | |-|
-| 31 | 2 |1.1.1|Replace({_pico_instance}, ["i1","68d4a766-4144-3248-aeb4-e212356716e4",1,"r1","e0df68c5-e7f9-395f-86b3-30ad9e1b7b07",["Offline",0],["Online",1],{b},"default"])|
-| 32 | 2 |1.1.2|Insert({_pico_replicaset}, ["r1","e0df68c5-e7f9-395f-86b3-30ad9e1b7b07","i1","default",0.0,"auto","not-ready"])|
-| 33 | 2 |1.1.3|Replace({_pico_instance}, ["i1","68d4a766-4144-3248-aeb4-e212356716e4",1,"r1","e0df68c5-e7f9-395f-86b3-30ad9e1b7b07",["Replicated",1],["Online",1],{b},"default"])|
-| 34 | 2 |1.1.4|Update({_pico_replicaset}, ["r1"], [["=","weight",1.0], ["=","state","ready"]])|
-| 35 | 2 |1.1.5|Replace({_pico_property}, ["target_vshard_config",[{{"e0df68c5-e7f9-395f-86b3-30ad9e1b7b07":[{{"68d4a766-4144-3248-aeb4-e212356716e4":["guest:@127.0.0.1:{p}","i1",true]}},1.0]}},"on"]])|
-| 36 | 2 |1.1.6|Replace({_pico_property}, ["current_vshard_config",[{{"e0df68c5-e7f9-395f-86b3-30ad9e1b7b07":[{{"68d4a766-4144-3248-aeb4-e212356716e4":["guest:@127.0.0.1:{p}","i1",true]}},1.0]}},"on"]])|
-| 37 | 2 |1.1.7|Replace({_pico_property}, ["vshard_bootstrapped",true])|
-| 38 | 2 |1.1.8|Replace({_pico_instance}, ["i1","68d4a766-4144-3248-aeb4-e212356716e4",1,"r1","e0df68c5-e7f9-395f-86b3-30ad9e1b7b07",["Online",1],["Online",1],{b},"default"])|
+| 18 | 1 |1.0.18|Insert({_pico_privilege}, ["usage","universe",0,1,1,0])|
+| 19 | 1 |1.0.19|Insert({_pico_privilege}, ["session","universe",0,1,1,0])|
+| 20 | 1 |1.0.20|Insert({_pico_privilege}, ["execute","role",2,0,1,0])|
+| 21 | 1 | |AddNode(1)|
+| 22 | 2 | |-|
+| 23 | 2 |1.1.1|Replace({_pico_instance}, ["i1","68d4a766-4144-3248-aeb4-e212356716e4",1,"r1","e0df68c5-e7f9-395f-86b3-30ad9e1b7b07",["Offline",0],["Online",1],{b},"default"])|
+| 24 | 2 |1.1.2|Insert({_pico_replicaset}, ["r1","e0df68c5-e7f9-395f-86b3-30ad9e1b7b07","i1","default",0.0,"auto","not-ready"])|
+| 25 | 2 |1.1.3|Replace({_pico_instance}, ["i1","68d4a766-4144-3248-aeb4-e212356716e4",1,"r1","e0df68c5-e7f9-395f-86b3-30ad9e1b7b07",["Replicated",1],["Online",1],{b},"default"])|
+| 26 | 2 |1.1.4|Update({_pico_replicaset}, ["r1"], [["=","weight",1.0], ["=","state","ready"]])|
+| 27 | 2 |1.1.5|Replace({_pico_property}, ["target_vshard_config",[{{"e0df68c5-e7f9-395f-86b3-30ad9e1b7b07":[{{"68d4a766-4144-3248-aeb4-e212356716e4":["guest:@127.0.0.1:{p}","i1",true]}},1.0]}},"on"]])|
+| 28 | 2 |1.1.6|Replace({_pico_property}, ["current_vshard_config",[{{"e0df68c5-e7f9-395f-86b3-30ad9e1b7b07":[{{"68d4a766-4144-3248-aeb4-e212356716e4":["guest:@127.0.0.1:{p}","i1",true]}},1.0]}},"on"]])|
+| 29 | 2 |1.1.7|Replace({_pico_property}, ["vshard_bootstrapped",true])|
+| 30 | 2 |1.1.8|Replace({_pico_instance}, ["i1","68d4a766-4144-3248-aeb4-e212356716e4",1,"r1","e0df68c5-e7f9-395f-86b3-30ad9e1b7b07",["Online",1],["Online",1],{b},"default"])|
+-----+----+-----+--------+
""".format( # noqa: E501
p=instance.port,
@@ -291,7 +283,7 @@ def test_raft_log(instance: Instance):
_pico_user=space_id("_pico_user"),
_pico_role=space_id("_pico_role"),
)
- assert strip_spaces(expected) == strip_spaces(raft_log)
+ assert strip_spaces(raft_log) == strip_spaces(expected)
def test_governor_notices_restarts(instance: Instance):
diff --git a/test/int/test_sql.py b/test/int/test_sql.py
index 960982c2b6..528192325c 100644
--- a/test/int/test_sql.py
+++ b/test/int/test_sql.py
@@ -936,8 +936,6 @@ def test_sql_alter_login(cluster: Cluster):
# * TODO: Check SESSION privilege is removed.
-# TODO: replace all Lua `i1.call`s to SQL `iq.sql`.
-# See https://git.picodata.io/picodata/picodata/sbroad/-/issues/492.
def test_sql_acl_privileges(cluster: Cluster):
cluster.deploy(instance_count=2)
i1, i2 = cluster.instances
@@ -977,9 +975,6 @@ def test_sql_acl_privileges(cluster: Cluster):
)
assert ddl["row_count"] == 1
- # Grant remote functions call.
- i1.grant_privilege(username, "execute", "universe", None)
-
# Remember number of default privileges.
default_privileges_number = len(
i1.sql(""" select * from "_pico_privilege" """)["rows"]
--
GitLab