Re: 44e692d0dbc: MDEV-29167 new db-level SHOW CREATE ROUTINE privilege
Hi, Oleksandr, On Sep 04, Oleksandr Byelkin wrote:
revision-id: 44e692d0dbc (mariadb-11.0.1-228-g44e692d0dbc) parent(s): 9cb75f333fa author: Oleksandr Byelkin committer: Oleksandr Byelkin timestamp: 2023-09-04 14:12:29 +0200 message:
MDEV-29167 new db-level SHOW CREATE ROUTINE privilege
-- Create general_log if CSV is enabled. diff --git a/scripts/mariadb_system_tables_fix.sql b/scripts/mariadb_system_tables_fix.sql --- a/scripts/mariadb_system_tables_fix.sql +++ b/scripts/mariadb_system_tables_fix.sql @@ -706,6 +706,9 @@ ALTER TABLE db modify Delete_history_priv enum('N','Y') COLLATE utf8mb3_general_
UPDATE user SET Delete_history_priv = Super_priv WHERE @had_user_delete_history_priv = 0;
+ALTER TABLE db ADD Show_create_routine_priv enum('N','Y') COLLATE utf8mb3_general_ci NOT NULL DEFAULT 'N' AFTER Delete_history_priv; +ALTER TABLE db modify Show_create_routine_priv enum('N','Y') COLLATE utf8mb3_general_ci NOT NULL DEFAULT 'N';
Why two statements?
+ ALTER TABLE user ADD plugin char(64) CHARACTER SET latin1 DEFAULT '' NOT NULL AFTER max_user_connections, ADD authentication_string TEXT NOT NULL AFTER plugin; ALTER TABLE user CHANGE auth_string authentication_string TEXT NOT NULL; diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc --- a/sql/sql_acl.cc +++ b/sql/sql_acl.cc @@ -9003,7 +9008,7 @@ bool check_routine_level_acl(THD *thd, const char *db, const char *name, NULL, db, sctx->priv_role, name, sph, 0))) - no_routine_acl= !(grant_proc->privs & SHOW_PROC_ACLS); + no_routine_acl= !(grant_proc->privs & SHOW_PROC_NO_DEF_ACLS);
Oh, now I get it. All the time up until this line I thought that SHOW_PROC_NO_DEF_ACLS means "privileges that *not a definer* of the routine needs to have" And now I see that it means to "show procedure without definition". Please, rename to something, like, SHOW_PROC_EXISTANCE or SHOW_PROC_NO_BODY or even SHOW_PROC_WITHOUT_DEFINITION. But SHOW_PROC_NO_DEF is rather misleading.
} mysql_rwlock_unlock(&LOCK_grant); return no_routine_acl; @@ -9237,7 +9242,7 @@ static const char *command_array[]= "CREATE USER", "EVENT", "TRIGGER", "CREATE TABLESPACE", "DELETE HISTORY", "SET USER", "FEDERATED ADMIN", "CONNECTION ADMIN", "READ_ONLY ADMIN", "REPLICATION SLAVE ADMIN", "REPLICATION MASTER ADMIN", "BINLOG ADMIN", - "BINLOG REPLAY", "SLAVE MONITOR" + "BINLOG REPLAY", "SLAVE MONITOR", "SHOW CREATE ROUTINE" };
static uint command_lengths[]= @@ -9250,7 +9255,7 @@ static uint command_lengths[]= 11, 5, 7, 17, 14, 8, 15, 16, 15, 23, 24, 12, - 13, 13 + 13, 13, 19
May be you'll change these two arrays into one of LEX_CSTRING's ? in a separate commit, of course.
};
diff --git a/sql/sql_show.cc b/sql/sql_show.cc --- a/sql/sql_show.cc +++ b/sql/sql_show.cc @@ -6676,7 +6677,8 @@ int store_schema_params(THD *thd, TABLE *table, TABLE *proc_table, DBUG_RETURN(0);
if (!full_access) - full_access= !strcmp(sp_user, definer.str); + full_access= !check_db_routine_access(thd, SHOW_CREATE_ROUTINE_ACL, + db.str, name.str, sph, TRUE);
Wait, I thought we agreed that to see a routine body one has to be either the definer or granted SHOW CREATE ROUTINE. As far as I can see check_db_routine_access doesn't check for the user being the definer.
if (!full_access && check_some_routine_access(thd, db.str, name.str, sph)) DBUG_RETURN(0);
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Hi Sergei!
Please review the deny refactoring commit in https://github.com/MariaDB/server/pull/2704. It introduces the exact lex_cstring change you suggested below here.
Vicențiu
On 5 September 2023 22:43:22 EEST, Sergei Golubchik via developers
Hi, Oleksandr,
On Sep 04, Oleksandr Byelkin wrote:
revision-id: 44e692d0dbc (mariadb-11.0.1-228-g44e692d0dbc) parent(s): 9cb75f333fa author: Oleksandr Byelkin committer: Oleksandr Byelkin timestamp: 2023-09-04 14:12:29 +0200 message:
MDEV-29167 new db-level SHOW CREATE ROUTINE privilege
-- Create general_log if CSV is enabled. diff --git a/scripts/mariadb_system_tables_fix.sql b/scripts/mariadb_system_tables_fix.sql --- a/scripts/mariadb_system_tables_fix.sql +++ b/scripts/mariadb_system_tables_fix.sql @@ -706,6 +706,9 @@ ALTER TABLE db modify Delete_history_priv enum('N','Y') COLLATE utf8mb3_general_
UPDATE user SET Delete_history_priv = Super_priv WHERE @had_user_delete_history_priv = 0;
+ALTER TABLE db ADD Show_create_routine_priv enum('N','Y') COLLATE utf8mb3_general_ci NOT NULL DEFAULT 'N' AFTER Delete_history_priv; +ALTER TABLE db modify Show_create_routine_priv enum('N','Y') COLLATE utf8mb3_general_ci NOT NULL DEFAULT 'N';
Why two statements?
+ ALTER TABLE user ADD plugin char(64) CHARACTER SET latin1 DEFAULT '' NOT NULL AFTER max_user_connections, ADD authentication_string TEXT NOT NULL AFTER plugin; ALTER TABLE user CHANGE auth_string authentication_string TEXT NOT NULL; diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc --- a/sql/sql_acl.cc +++ b/sql/sql_acl.cc @@ -9003,7 +9008,7 @@ bool check_routine_level_acl(THD *thd, const char *db, const char *name, NULL, db, sctx->priv_role, name, sph, 0))) - no_routine_acl= !(grant_proc->privs & SHOW_PROC_ACLS); + no_routine_acl= !(grant_proc->privs & SHOW_PROC_NO_DEF_ACLS);
Oh, now I get it. All the time up until this line I thought that SHOW_PROC_NO_DEF_ACLS means "privileges that *not a definer* of the routine needs to have"
And now I see that it means to "show procedure without definition".
Please, rename to something, like, SHOW_PROC_EXISTANCE or SHOW_PROC_NO_BODY or even SHOW_PROC_WITHOUT_DEFINITION. But SHOW_PROC_NO_DEF is rather misleading.
} mysql_rwlock_unlock(&LOCK_grant); return no_routine_acl; @@ -9237,7 +9242,7 @@ static const char *command_array[]= "CREATE USER", "EVENT", "TRIGGER", "CREATE TABLESPACE", "DELETE HISTORY", "SET USER", "FEDERATED ADMIN", "CONNECTION ADMIN", "READ_ONLY ADMIN", "REPLICATION SLAVE ADMIN", "REPLICATION MASTER ADMIN", "BINLOG ADMIN", - "BINLOG REPLAY", "SLAVE MONITOR" + "BINLOG REPLAY", "SLAVE MONITOR", "SHOW CREATE ROUTINE" };
static uint command_lengths[]= @@ -9250,7 +9255,7 @@ static uint command_lengths[]= 11, 5, 7, 17, 14, 8, 15, 16, 15, 23, 24, 12, - 13, 13 + 13, 13, 19
May be you'll change these two arrays into one of LEX_CSTRING's ? in a separate commit, of course.
};
diff --git a/sql/sql_show.cc b/sql/sql_show.cc --- a/sql/sql_show.cc +++ b/sql/sql_show.cc @@ -6676,7 +6677,8 @@ int store_schema_params(THD *thd, TABLE *table, TABLE *proc_table, DBUG_RETURN(0);
if (!full_access) - full_access= !strcmp(sp_user, definer.str); + full_access= !check_db_routine_access(thd, SHOW_CREATE_ROUTINE_ACL, + db.str, name.str, sph, TRUE);
Wait, I thought we agreed that to see a routine body one has to be either the definer or granted SHOW CREATE ROUTINE.
As far as I can see check_db_routine_access doesn't check for the user being the definer.
if (!full_access && check_some_routine_access(thd, db.str, name.str, sph)) DBUG_RETURN(0);
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org _______________________________________________ developers mailing list -- developers@lists.mariadb.org To unsubscribe send an email to developers-leave@lists.mariadb.org
Hi, Sergei!
On Tue, Sep 5, 2023 at 9:43 PM Sergei Golubchik
Hi, Oleksandr,
On Sep 04, Oleksandr Byelkin wrote:
revision-id: 44e692d0dbc (mariadb-11.0.1-228-g44e692d0dbc) parent(s): 9cb75f333fa author: Oleksandr Byelkin committer: Oleksandr Byelkin timestamp: 2023-09-04 14:12:29 +0200 message:
MDEV-29167 new db-level SHOW CREATE ROUTINE privilege
-- Create general_log if CSV is enabled. diff --git a/scripts/mariadb_system_tables_fix.sql b/scripts/mariadb_system_tables_fix.sql --- a/scripts/mariadb_system_tables_fix.sql +++ b/scripts/mariadb_system_tables_fix.sql @@ -706,6 +706,9 @@ ALTER TABLE db modify Delete_history_priv enum('N','Y') COLLATE utf8mb3_general_
UPDATE user SET Delete_history_priv = Super_priv WHERE @had_user_delete_history_priv = 0;
+ALTER TABLE db ADD Show_create_routine_priv enum('N','Y') COLLATE utf8mb3_general_ci NOT NULL DEFAULT 'N' AFTER Delete_history_priv; +ALTER TABLE db modify Show_create_routine_priv enum('N','Y') COLLATE utf8mb3_general_ci NOT NULL DEFAULT 'N';
Why two statements?
I also wonder about this pattern, but then I think I got the idea, 1 add the field if it was absolutely absent, 2. fix properties if they were incorrect. But mostly I follow the pattern of other fields adding.
+ ALTER TABLE user ADD plugin char(64) CHARACTER SET latin1 DEFAULT '' NOT NULL AFTER max_user_connections, ADD authentication_string TEXT NOT NULL AFTER plugin; ALTER TABLE user CHANGE auth_string authentication_string TEXT NOT NULL; diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc --- a/sql/sql_acl.cc +++ b/sql/sql_acl.cc @@ -9003,7 +9008,7 @@ bool check_routine_level_acl(THD *thd, const char *db, const char *name, NULL, db, sctx->priv_role, name, sph, 0))) - no_routine_acl= !(grant_proc->privs & SHOW_PROC_ACLS); + no_routine_acl= !(grant_proc->privs & SHOW_PROC_NO_DEF_ACLS);
Oh, now I get it. All the time up until this line I thought that SHOW_PROC_NO_DEF_ACLS means "privileges that *not a definer* of the routine needs to have"
And now I see that it means to "show procedure without definition".
Please, rename to something, like, SHOW_PROC_EXISTANCE or SHOW_PROC_NO_BODY or even SHOW_PROC_WITHOUT_DEFINITION. But
SHOW_PROC_NO_BODY was my first idea, but BODY used in other sens (package for example) so I will go with SHOW_PROC_WITHOUT_DEFINITION
SHOW_PROC_NO_DEF is rather misleading.
} mysql_rwlock_unlock(&LOCK_grant); return no_routine_acl; @@ -9237,7 +9242,7 @@ static const char *command_array[]= "CREATE USER", "EVENT", "TRIGGER", "CREATE TABLESPACE", "DELETE HISTORY", "SET USER", "FEDERATED ADMIN", "CONNECTION ADMIN", "READ_ONLY ADMIN", "REPLICATION SLAVE ADMIN", "REPLICATION MASTER ADMIN", "BINLOG ADMIN", - "BINLOG REPLAY", "SLAVE MONITOR" + "BINLOG REPLAY", "SLAVE MONITOR", "SHOW CREATE ROUTINE" };
static uint command_lengths[]= @@ -9250,7 +9255,7 @@ static uint command_lengths[]= 11, 5, 7, 17, 14, 8, 15, 16, 15, 23, 24, 12, - 13, 13 + 13, 13, 19
May be you'll change these two arrays into one of LEX_CSTRING's ? in a separate commit, of course.
OK
};
diff --git a/sql/sql_show.cc b/sql/sql_show.cc --- a/sql/sql_show.cc +++ b/sql/sql_show.cc @@ -6676,7 +6677,8 @@ int store_schema_params(THD *thd, TABLE *table, TABLE *proc_table, DBUG_RETURN(0);
if (!full_access) - full_access= !strcmp(sp_user, definer.str); + full_access= !check_db_routine_access(thd, SHOW_CREATE_ROUTINE_ACL, + db.str, name.str, sph, TRUE);
Wait, I thought we agreed that to see a routine body one has to be either the definer or granted SHOW CREATE ROUTINE.
As far as I can see check_db_routine_access doesn't check for the user being the definer.
yes, I have just accidently lost the condition (will add also tests for it)
if (!full_access && check_some_routine_access(thd, db.str, name.str, sph)) DBUG_RETURN(0);
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Hi, Oleksandr, On Sep 06, Oleksandr Byelkin wrote:
+ALTER TABLE db ADD Show_create_routine_priv enum('N','Y') COLLATE utf8mb3_general_ci NOT NULL DEFAULT 'N' AFTER Delete_history_priv; +ALTER TABLE db modify Show_create_routine_priv enum('N','Y') COLLATE utf8mb3_general_ci NOT NULL DEFAULT 'N';
Why two statements?
I also wonder about this pattern, but then I think I got the idea, 1 add the field if it was absolutely absent, 2. fix properties if they were incorrect. But mostly I follow the pattern of other fields adding.
Well, yes, almost. The second statement is, indeed, to fix the column if it existed with incorrect definition. But it's when the definition is *changed in a later version*. For example, if you'd add in 11.3 ALTER TABLE db ADD Show_create_routine_priv enum('N','Y') COLLATE utf8mb3_general_ci NOT NULL DEFAULT 'N' AFTER Delete_history_priv; and, hypothetically, in 11.4 we'd want to use utf8mb4_general_ci, then in 11.4 it'd be two statements: ALTER TABLE db ADD Show_create_routine_priv enum('N','Y') COLLATE utf8mb4_general_ci NOT NULL DEFAULT 'N' AFTER Delete_history_priv; ALTER TABLE db modify Show_create_routine_priv enum('N','Y') COLLATE utf8mb4_general_ci NOT NULL DEFAULT 'N'; where the first statement would work for upgrades from 11.2 and below, and the second statement would work for upgrades from 11.3. But there's no need to do that when you add a new column for the first time.
@@ -9250,7 +9255,7 @@ static uint command_lengths[]= 11, 5, 7, 17, 14, 8, 15, 16, 15, 23, 24, 12, - 13, 13 + 13, 13, 19
May be you'll change these two arrays into one of LEX_CSTRING's ? in a separate commit, of course.
OK
No, please don't, Vicentiu replied that he already did it the DENY branch. Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
participants (3)
-
Oleksandr Byelkin -
Sergei Golubchik -
Vicențiu Ciorbaru