Hi Kristofer,
I think some work still has to be done in the scope of this bug. Please see
my notes below.
> ChangeSet@stripped, 2008-04-29 13:36:29+02:00, thek@adventure.(none) +7 -0
> Bug#31501 Stored Routines: droping stored procedure revokes all assotiated
> privileges
>
> When a routine was dropped all associated privileges were removed even
> though they weren't associated with the definer of the routine.
>
> This patch changes the behavior so that only the automatic privileges set
> by the definer of the routine are dropped when the routine is dropped.
Okay, first of all, let's clarify what we're going to achieve.
1. The Manual says:
- The ALTER ROUTINE privilege is needed to alter or drop stored routines.
This privilege is granted automatically to the creator of a routine if
necessary, and dropped when the routine creator drops the routine.
- The EXECUTE privilege is required to execute stored routines. However,
this privilege is granted automatically to the creator of a routine if
necessary (and dropped when the creator drops the routine).
- If the automatic_sp_privileges system variable is 0, the EXECUTE and
ALTER ROUTINE privileges are not automatically granted and dropped.
First of all, I believe, 'the creator of a routine' is 'the definer of a
routine', because we don't have a notion of 'a creator' rather than
definer. I would ask docs-team to explicitly clarify that.
So, that means the following:
(expect automatic_sp_privileges = ON)
- CREATE PROCEDURE p1() SELECT 1;
-> GRANT EXECUTE, ALTER ON db.p1 TO <current user>
(since definer is the current user)
- CREATE DEFINER=a@b PROCEDURE p1() SELECT 1;
-> GRANT EXECUTE, ALTER ON db.p1 TO a@b
(since definer is a@b)
(and frankly speaking, your patch does not work that way AFAICS. Also, I'd
add such a tests to the test case)
Then, the question is -- what about SQL SECURITY INVOKER? What is the
reason to automatically grant access for the definer to stored routine
declared as SQL SECURITY INVOKER? Should we really do that? I'd clarify
that with PeterG and/or architects.
Finally, what if a stored routine was created with
automatic_sp_privileges == OFF and then dropped with
automatic_sp_privileges == ON? I guess, the user privileges will be
dropped. This is a way for the user to shoot himself in the foot.
So, I'd:
- request to clarify that in the manual;
- rephrase your second statement (This patch changes the behavior so
that only the automatic privileges set by the definer of the routine
are dropped when the routine is dropped) -- it is *not* only automatic
privileges.
> sql/sp.cc@stripped, 2008-04-29 13:36:25+02:00, thek@adventure.(none) +15 -1
> Remember the definer field of the target SP. This information is later
> used when revoking privileges set by automatic_sp_privileges.
>
> sql/sp.h@stripped, 2008-04-29 13:36:25+02:00, thek@adventure.(none) +1 -1
> Changed signature of sp_drop_routine
>
> sql/sql_acl.cc@stripped, 2008-04-29 13:36:25+02:00, thek@adventure.(none) +51 -20
> Revoke only those privileges which correspond to the definer of the
> stored program.
>
> sql/sql_acl.h@stripped, 2008-04-29 13:36:25+02:00, thek@adventure.(none) +1 -1
> Changed signature of sp_revoke_privileges
>
> sql/sql_parse.cc@stripped, 2008-04-29 13:36:25+02:00, thek@adventure.(none) +21 -20
> In order to avoid revoking privileges from other users than
> the sp definer the order of execution has changed.
> 1. Drop SP (remember definer) 2. Revoke SP privileges
>
> diff -Nrup a/mysql-test/t/sp-destruct.test b/mysql-test/t/sp-destruct.test
> --- a/mysql-test/t/sp-destruct.test 2007-08-07 11:40:51 +02:00
> +++ b/mysql-test/t/sp-destruct.test 2008-04-29 13:36:25 +02:00
> @@ -154,3 +154,48 @@ drop procedure bug14233_3;
> # Assert: These should show nothing.
> show procedure status;
> show function status;
> +
> +
> +--echo #
> +--echo # Bug#31501 Stored Routines: droping stored procedure revokes all assotiated
> privileges
> +--echo #
> +
> +SET GLOBAL automatic_sp_privileges=ON;
Please remember the old value of automatic_sp_privileges and reset it to that
value in the end of the test case.
> +DROP PROCEDURE IF EXISTS test.bug31501_proc1;
You'd better wrap this statement with disable_warnings / enable_warnings
directives.
Also, please do not use absolute name (like <database name>.<table name>).
Another thing -- generally, the test suite tries to use "commmon" names
just to make minimal impact on user data (if/when the test suite is run on
the user database). So, please use names like p1, p2 for procedures, and
mysqltest_1, mysqltest_2 for user names.
> +delimiter //;
> +create procedure bug31501_proc1 (OUT param1 INT) BEGIN
> + SELECT COUNT(*) INTO param1 FROM t;
> +END; //
> +delimiter ;//
Basically, no need to define a compound statement here. This procedure can
be written just as:
CREATE PROCEDURE bug31501_proc1(OUT param1 INT)
SELECT COUNT(*) INTO param1 FROM t;
> +--echo ** Grant CREATE ROUTINE to user bug31501_user.
I consider dumping debug information into result file very useful. May be
you should also dump a new line before a new logiclal block. Something like
that:
--echo
--echo # -- Do this
...
--echo
--echo # -- Do that
...
> +GRANT CREATE ROUTINE ON test.* TO 'bug31501_user'@'localhost';
> +SHOW GRANTS FOR 'bug31501_user'@'localhost';
> +
> +--echo ** Connecting as bug31501_user
> +--connect (bug31501_con,localhost,bug31501_user,,test,,)
> +--connection bug31501_con
> +delimiter //;
> +create procedure bug31501_proc2 (OUT param1 INT) BEGIN
> + SELECT COUNT(*) INTO param1 FROM t;
> +END; //
> +delimiter ;//
> +--echo ** Creating SP and get automatic privileges (gaining EXECUTE,ALTER ROUTINE)
> +SHOW GRANTS FOR 'bug31501_user'@'localhost';
> +
> +--connection default
I find it quite useful to dump some debug message while switching
connection. Something like:
--echo
--echo # -- connection: default
> +--echo ** Dropping procedure with definer=root
> +--echo ** This won't result in any grant changes because we don't remove grants from
> ther users automatically.
typo (ther).
> +DROP PROCEDURE test.bug31501_proc1;
> +SHOW GRANTS FOR 'bug31501_user'@'localhost';
> +
> +--echo ** Not dropping procedure as definer=bug31501_user
I don't get this comment. What do you mean by 'Not droppping'?
> +DROP PROCEDURE test.bug31501_proc2;
> +--echo ** Execution grants for bug31501_user should now be revoked.
> +SHOW GRANTS FOR 'bug31501_user'@'localhost';
> +--connection default
Err... The connection is already default, isn't it?
> +--disconnect bug31501_con
> +SET GLOBAL automatic_sp_privileges=DEFAULT;
> +
> diff -Nrup a/sql/sp.cc b/sql/sp.cc
> --- a/sql/sp.cc 2008-04-08 18:35:58 +02:00
> +++ b/sql/sp.cc 2008-04-29 13:36:25 +02:00
> @@ -956,13 +956,15 @@ done:
> @param type Stored routine type
> (TYPE_ENUM_PROCEDURE or TYPE_ENUM_FUNCTION)
> @param name Stored routine name.
> + @param[out] definer The definer of the stored proceedure if it was found or
> + null.
>
> @return Error code. SP_OK is returned on success. Other SP_ constants are
> used to indicate about errors.
> */
>
> int
> -sp_drop_routine(THD *thd, int type, sp_name *name)
> +sp_drop_routine(THD *thd, int type, sp_name *name, String *sp_definer)
> {
> TABLE *table;
> int ret;
> @@ -984,6 +986,14 @@ sp_drop_routine(THD *thd, int type, sp_n
> DBUG_RETURN(SP_OPEN_TABLE_FAILED);
> if ((ret= db_find_routine_aux(thd, type, name, table)) == SP_OK)
> {
> + bool no_such_field= get_field(thd->mem_root,
> + table->field[MYSQL_PROC_FIELD_DEFINER],
> + sp_definer);
> + if (no_such_field)
> + {
> + ret= SP_DELETE_ROW_FAILED;
> + goto error;
> + }
Although personally I hate goto, I can not insist on not using it. :)
> if (table->file->ha_delete_row(table->record[0]))
> ret= SP_DELETE_ROW_FAILED;
> }
> @@ -994,6 +1004,7 @@ sp_drop_routine(THD *thd, int type, sp_n
> sp_cache_invalidate();
> }
>
> +error:
> close_thread_tables(thd);
> DBUG_RETURN(ret);
> }
> @@ -1349,7 +1360,9 @@ sp_exist_routines(THD *thd, TABLE_LIST *
> parsing the definition. (Used for dropping).
>
> @param thd thread context
> + @param type TYPE_ENUM_PROCEDURE or TYPE_ENUM_FUNCTION
> @param name name of procedure
> + @param sp_definer[out] The user name of the
Why did you add a comment for sp_definer?
> @retval
> 0 Success
> @@ -1372,6 +1385,7 @@ sp_routine_exists_in_table(THD *thd, int
> ret= SP_KEY_NOT_FOUND;
> close_system_tables(thd, &open_tables_state_backup);
> }
> +
> return ret;
> }
Unnecessary change?
> diff -Nrup a/sql/sql_acl.cc b/sql/sql_acl.cc
> --- a/sql/sql_acl.cc 2008-04-14 13:30:04 +02:00
> +++ b/sql/sql_acl.cc 2008-04-29 13:36:25 +02:00
> @@ -6166,21 +6166,25 @@ Silence_routine_definer_errors::handle_e
> Revoke privileges for all users on a stored procedure. Use an error handler
> that converts errors about missing grants into warnings.
>
> - @param
> - thd The current thread.
> - @param
> - db DB of the stored procedure
> - @param
> - name Name of the stored procedure
> + This method works in conjuction with the automatic_sp_privileges system
> + variable and it demands that this variable is set to ON.
>
> - @retval
> - 0 OK.
> - @retval
> - < 0 Error. Error message not yet sent.
> + The method only removes privilages related to the user which created the sp
> + and got automatic privileges.
> +
> + @param thd The current thread.
> + @param sp_db DB of the stored procedure
> + @param sp_name Name of the stored procedure
> + @param is_proc TRUE if the named sp is a PROCEDURE
> + @param definer_user Contains username@host of the sp definer.
> +
> + @return Did an error occur?
> + @retval FALSE OK.
> + @retval TRUE Error. Error message not yet sent.
> */
>
> bool sp_revoke_privileges(THD *thd, const char *sp_db, const char *sp_name,
> - bool is_proc)
> + bool is_proc, String *definer_user)
> {
> uint counter, revoked;
> int result;
> @@ -6189,15 +6193,17 @@ bool sp_revoke_privileges(THD *thd, cons
> Silence_routine_definer_errors error_handler;
> DBUG_ENTER("sp_revoke_privileges");
>
> + if (definer_user->length() <1)
> + DBUG_RETURN(0);
How come the definer can be NULL?
> if ((result= open_grant_tables(thd, tables)))
> DBUG_RETURN(result != 1);
>
> - /* Be sure to pop this before exiting this scope! */
> - thd->push_internal_handler(&error_handler);
> -
> rw_wrlock(&LOCK_grant);
> VOID(pthread_mutex_lock(&acl_cache->lock));
> -
> + /*
> + Be sure to pop this before exiting this scope!
> + */
> + thd->push_internal_handler(&error_handler);
Why did you move that code into a critical section?
/*
This statement will be replicated as a statement, even when using
row-based replication. The flag will be reset at the end of the
@@ -6205,14 +6211,30 @@ bool sp_revoke_privileges(THD *thd, cons
*/
thd->clear_current_stmt_binlog_row_based();
- /* Remove procedure access */
do
{
+ String at_sign;
+ at_sign.append("@");
+ String grant_proc_user;
+ String grant_proc_host;
+ int offset= definer_user->strstr(at_sign,0);
1. I think, it's an overkill to use our String to do just a strstr().
2. What is more important is that username can contain '@'-character. In
this case your search will not work. Please use parse_user() function,
which is intended exactly for this purpose.
When you do that, I think the code below will change, so I'll not comment
on it.
> diff -Nrup a/sql/sql_parse.cc b/sql/sql_parse.cc
> --- a/sql/sql_parse.cc 2008-04-18 09:17:46 +02:00
> +++ b/sql/sql_parse.cc 2008-04-29 13:36:25 +02:00
> @@ -4255,15 +4255,16 @@ create_sp_error:
> case SQLCOM_DROP_FUNCTION:
> {
> int sp_result;
> - int type= (lex->sql_command == SQLCOM_DROP_PROCEDURE ?
> - TYPE_ENUM_PROCEDURE : TYPE_ENUM_FUNCTION);
> -
> + String sp_definer;
> + char *db= lex->spname->m_db.str;
> + char *name= lex->spname->m_name.str;
> + int type= lex->sql_command == SQLCOM_DROP_PROCEDURE ?
> + TYPE_ENUM_PROCEDURE :
> + TYPE_ENUM_FUNCTION;
> sp_result= sp_routine_exists_in_table(thd, type, lex->spname);
> mysql_reset_errors(thd, 0);
> if (sp_result == SP_OK)
> {
> - char *db= lex->spname->m_db.str;
> - char *name= lex->spname->m_name.str;
>
> if (check_routine_access(thd, ALTER_PROC_ACL, db, name,
> lex->sql_command == SQLCOM_DROP_PROCEDURE, 0))
> @@ -4271,23 +4272,10 @@ create_sp_error:
>
> if (end_active_trans(thd))
> goto error;
> -#ifndef NO_EMBEDDED_ACCESS_CHECKS
> - if (sp_automatic_privileges && !opt_noacl &&
> - sp_revoke_privileges(thd, db, name,
> - lex->sql_command == SQLCOM_DROP_PROCEDURE))
> - {
> - push_warning(thd, MYSQL_ERROR::WARN_LEVEL_WARN,
> - ER_PROC_AUTO_REVOKE_FAIL,
> - ER(ER_PROC_AUTO_REVOKE_FAIL));
> - }
> -#endif
> - /* Conditionally writes to binlog */
>
> - int type= lex->sql_command == SQLCOM_DROP_PROCEDURE ?
> - TYPE_ENUM_PROCEDURE :
> - TYPE_ENUM_FUNCTION;
> + /* Conditionally writes to binlog */
>
> - sp_result= sp_drop_routine(thd, type, lex->spname);
> + sp_result= sp_drop_routine(thd, type, lex->spname, &sp_definer);
> }
> else
> {
> @@ -4318,6 +4306,19 @@ create_sp_error:
> }
> }
> res= sp_result;
> +
> +#ifndef NO_EMBEDDED_ACCESS_CHECKS
> + if (sp_automatic_privileges && !opt_noacl &&
> + sp_revoke_privileges(thd, db, name,
> + lex->sql_command == SQLCOM_DROP_PROCEDURE,
> + &sp_definer))
> + {
> + push_warning(thd, MYSQL_ERROR::WARN_LEVEL_WARN,
> + ER_PROC_AUTO_REVOKE_FAIL,
> + ER(ER_PROC_AUTO_REVOKE_FAIL));
> + }
> +#endif
> +
> switch (sp_result) {
> case SP_OK:
> my_ok(thd);
--
Alexander Nozdrin, Software Developer
MySQL AB, Moscow, Russia, www.mysql.com
