From 967718da793b30bec39b4b1a9fb6d0c979c4d3f9 Mon Sep 17 00:00:00 2001
From: Georgy Moshkin <gmoshkin@picodata.io>
Date: Wed, 25 Sep 2024 17:45:21 +0300
Subject: [PATCH] fix: used to panic when attempting an unauthorized plugin
operation
---
src/access_control.rs | 17 +++++++++----
src/sql.rs | 10 ++++++++
test/int/test_plugin.py | 53 +++++++++++++++++++++++++++++++++++++++++
3 files changed, 75 insertions(+), 5 deletions(-)
diff --git a/src/access_control.rs b/src/access_control.rs
index 0a21d301d8..ad9cd17ecc 100644
--- a/src/access_control.rs
+++ b/src/access_control.rs
@@ -667,17 +667,24 @@ pub(super) fn access_check_op(
Ok(())
}
Op::Plugin { .. } => {
- if !is_superuser(as_user) {
- let sys_user = user_by_id(as_user)?;
- #[rustfmt::skip]
- return Err(BoxError::new(AccessDenied, format!("Plugin system access is denied for user '{}'", sys_user.name)).into());
- }
+ access_check_plugin_system(as_user)?;
Ok(())
}
Op::Acl(acl) => access_check_acl(storage, acl, as_user),
}
}
+/// This function is also called from [`crate::sql::dispatch`] for client-side
+/// permission checking. See comments next to it for details.
+pub(crate) fn access_check_plugin_system(as_user: UserId) -> tarantool::Result<()> {
+ if !is_superuser(as_user) {
+ let sys_user = user_by_id(as_user)?;
+ #[rustfmt::skip]
+ return Err(BoxError::new(AccessDenied, format!("Plugin system access is denied for user '{}'", sys_user.name)).into());
+ }
+ Ok(())
+}
+
// TODO: use this function everywhere we check `id != ADMIN_ID`
#[inline(always)]
pub fn is_superuser(user_id: UserId) -> bool {
diff --git a/src/sql.rs b/src/sql.rs
index 6b87de06ab..8a9f527055 100644
--- a/src/sql.rs
+++ b/src/sql.rs
@@ -1,5 +1,6 @@
//! Clusterwide SQL query execution.
+use crate::access_control::access_check_plugin_system;
use crate::access_control::{validate_password, UserMetadataKind};
use crate::cas::Predicate;
use crate::schema::{
@@ -242,6 +243,15 @@ pub fn dispatch(mut query: Query<RouterRuntime>) -> traft::Result<Tuple> {
Ok(duration_from_secs_f64_clamped(secs))
};
+ // NOTE: this is different from how access checks are done for DDL, because:
+ // 1) We do not plan on allowing non-superusers to do plugin operations in the near future.
+ // 2) Preparing a plugin operation involves accessing a number of
+ // internal tables and potentially even running plugin code for
+ // validation purposes, so we exit early in case of denied access.
+ // See also <https://git.picodata.io/picodata/picodata/picodata/-/issues/965>
+ let as_user = effective_user_id();
+ with_su(ADMIN_ID, || access_check_plugin_system(as_user))??;
+
match plugin {
Plugin::Create(CreatePlugin {
name,
diff --git a/test/int/test_plugin.py b/test/int/test_plugin.py
index bfb7699204..f315a669a5 100644
--- a/test/int/test_plugin.py
+++ b/test/int/test_plugin.py
@@ -2735,3 +2735,56 @@ def test_sql_interface_inheritance(cluster: Cluster):
plugin_ref.assert_synced()
cfg_space = plugin_ref.get_config("testservice_1", i1)
assert cfg_space == {"foo": True, "bar": 101, "baz": ["one", "two", "three"]}
+
+
+def test_plugin_sql_permission_denied(cluster: Cluster):
+ [i1] = cluster.deploy(instance_count=1)
+
+ with pytest.raises(TarantoolError) as e:
+ i1.sql(f"CREATE PLUGIN {_PLUGIN} 0.1.0", user="guest")
+ assert e.value.args[:2] == (
+ "ER_ACCESS_DENIED",
+ "Plugin system access is denied for user 'guest'",
+ )
+
+ # Access is checked before plugin name validation
+ with pytest.raises(TarantoolError) as e:
+ i1.sql("CREATE PLUGIN no_such_plugin 0.1.0", user="guest")
+ assert e.value.args[:2] == (
+ "ER_ACCESS_DENIED",
+ "Plugin system access is denied for user 'guest'",
+ )
+
+ # Create as superuser to check the rest of commands
+ i1.sql(f"CREATE PLUGIN {_PLUGIN} 0.1.0")
+
+ with pytest.raises(TarantoolError) as e:
+ i1.sql(
+ f'ALTER PLUGIN {_PLUGIN} 0.1.0 ADD SERVICE no_such_service TO TIER "{_DEFAULT_TIER}"',
+ user="guest",
+ )
+ assert e.value.args[:2] == (
+ "ER_ACCESS_DENIED",
+ "Plugin system access is denied for user 'guest'",
+ )
+
+ with pytest.raises(TarantoolError) as e:
+ i1.sql(f"ALTER PLUGIN {_PLUGIN} 0.1.0 ENABLE", user="guest")
+ assert e.value.args[:2] == (
+ "ER_ACCESS_DENIED",
+ "Plugin system access is denied for user 'guest'",
+ )
+
+ with pytest.raises(TarantoolError) as e:
+ i1.sql(f"ALTER PLUGIN {_PLUGIN} 0.1.0 DISABLE", user="guest")
+ assert e.value.args[:2] == (
+ "ER_ACCESS_DENIED",
+ "Plugin system access is denied for user 'guest'",
+ )
+
+ with pytest.raises(TarantoolError) as e:
+ i1.sql(f"DROP PLUGIN {_PLUGIN} 0.1.0", user="guest")
+ assert e.value.args[:2] == (
+ "ER_ACCESS_DENIED",
+ "Plugin system access is denied for user 'guest'",
+ )
--
GitLab