Hi Chuck,
as promised, I looked through the patch again. I didn't find anything,
which would make me to withdraw my approval.
However, I have some questions and/or comments to parts of the patch,
which I either did already mention before, or have missed before.
Since everything below is on the suggestion/option/commentary level, I
did not divide them into the usual sections.
Chuck Bell, 06.11.2009 15:47:
...
> 2888 Chuck Bell 2009-11-06
> BUG#44787 : Backup: Check privileges before executing BACKUP/RESTORE
>
> Restore can fail in the middle if the user does not have
> sufficient privileges to create and populate all of the
> objects.
>
> This patch implements a privilege precheck step to check
> all objects for proper access. If any object fails the
> privilege check, restore halts with an error.
>
> The object-level privileges checked include the following.
>
> RESTORE,CREATE,DROP on db.*
> CREATE on db.x (if table or view x)
> CREATE_TABLESPACE on *.* (if tablespace)
> SUPER on *.* (if view, stored routine, event or trigger)
> CREATE_PROC on db.* (if stored routine)
> EVENT on db.* (if event)
> GRANT on db.* (if privilege)
> TRIGGER on db.* (if trigger but table not found)
> TRIGGER on db.t (if trigger on t)
The latter two look ok from the privileges point of view. But if "table
not found", won't RESTORE fail anyway? Is it possible to create a
trigger on a non-existent table?
I mean, this work is not meant to catch all possible RESTORE problems in
advance. But if we get a check for one of the non-privilege based
problems for free, why not use it?
>
> Note: This is patch 1 of 3. Patch 2 implements elevation,
> patch 3 implements the options to skip precheck and turn
> elevation off.
...
> === modified file 'mysql-test/suite/backup/include/error_name_to_number.inc'
...
> --LET $ER_BACKUP_ACCESS_DENIED_ERROR = 1793
> --LET $ER_BACKUP_ACCESS_DBS_INCOMPLETE = 1795
> --LET $ER_BACKUP_ACCESS_OBJS_INCOMPLETE = 1796
> +--LET $ER_RESTORE_DB_ERROR = 1800
Not related to this patch, but anyway:
I haven't seen this concept before. We maintain an error number list
manually here? Won't these numbers need to be changed after a merge with
the main repository (due to conflicts in errmsg-utf8.txt)?
If I understand correctly, this approach is required because --error
does not evaluate its arguments? Has it been considered, if extending
mysqltest was an option?
...
> === added file 'mysql-test/suite/backup/t/backup_restore_security.test'
...
> +# Test Cases
> +# ----------
> +# 1. Ensure a user with only RESTORE on db1.* cannot restore the database.
> +# 2. Ensure a user with only SELECT on db1.* cannot restore the database.
I wonder, what this test case #2 tries to prove. It doesn't sound like a
great achievement, that we reject RESTORE if only SELECT_ACL is present.
Whom do we want to say "wow" that a database, which is read-only for a
user, cannot be dropped and re-created by him?
IMHO it would make much more sense to show that ALL privileges *but*
RESTORE_ACL don't work. This would even more show that RESTORE_ACL is
*required*.
> +# 3. Show that SUPER is needed when there are object that use a definer
> +# clause are in the backup image.
> +# 4. Ensure a user can only restore the database if and only if she has
> +# all of the required permissions to create and populate all objects.
That test case #4 is fine. But wouldn't it be great, if we show that
RESTORE fails before it touches the objects? That RESTORE fails with
insufficient privileges is not new. This works even without this patch.
But the new thing that we detect it early. That's the main difference. I
think we should show that. Juts list the objects after the failed
RESTORE. They should all be present.
...
> +--echo #
> +--echo # Test Case 3 : Show that SUPER is needed to complete restore
> +--echo # when there are objects with definer clauses.
> +--echo #
...
> +REVOKE ALL ON *.* FROM 'bup_some_priv'@'localhost';
> +GRANT ALL ON backup_test.* TO 'bup_some_priv'@'localhost' WITH GRANT OPTION;
> +GRANT ALL ON mysql.* TO 'bup_some_priv'@'localhost';
...
> +--error ER_RESTORE_ACCESS_DEFINER
> +RESTORE FROM 'backup_test_no_proc.bak' OVERWRITE;
> +--replace_column 2 #
> +SHOW ERRORS;
Frankly, this doesn't show what the heading says. It just show that ALL
on the database plus ALL on mysql.* is insufficient. This leaves room
for speculation, which global privilege(s) might be needed.
I strongly suggest to extend this test case by adding SUPER and show
that this alone makes RESTORE succeed.
> +
> +--echo #
> +--echo # Test Case 4 : Ensure a user can only restore the database if and only
> +--echo # if she has all of the required permissions to create and
> +--echo # populate all objects.
> +--echo #
...
> +REVOKE ALL ON *.* FROM 'bup_some_priv'@'localhost';
> +GRANT ALL ON backup_test.* TO 'bup_some_priv'@'localhost' WITH GRANT OPTION;
> +GRANT SELECT ON mysql.* TO 'bup_some_priv'@'localhost';
> +GRANT SUPER ON *.* TO 'bup_some_priv'@'localhost';
> +
...
> +--replace_column 1 #
> +RESTORE FROM 'backup_test_orig.bak' OVERWRITE;
Again, this doesn't match the title either. It shows that RESTORE is
possible with a broad set of privileges. It definitely does not prove
the truth of "only ... and only if" of the title. To prove it, we would
need to revoke one privilege after the other and thus show that a single
one missing from the required set makes RESTORE fail.
Or adapt the title to what the test really proves.
...
> +--echo #
> +--echo # Test Case 5 : Show that the privilege check on TRIGGER is required to
> +--echo # restore a table with a trigger.
Do you mean that the privilege itself is required, or indeed a *check*
of the privilege is required to restore?
> +
> +--echo #
> +--echo # Perform a backup with only a table and a trigger.
> +--echo # No grants either.
> +--echo #
> +
> +REVOKE ALL ON *.* FROM 'bup_some_priv'@'localhost';
> +REVOKE ALL ON backup_test.* FROM 'bup_some_priv'@'localhost';
> +DROP USER 'bup_some_priv'@'localhost';
Are you sure that these revokes are required before a DROP USER? If so,
I'd deem it a bug. DROP USER has to eliminate all traces of privileges
for the user, except in the hosts table, which we ignore so far anyway.
...
> +CREATE USER 'bup_some_priv'@'localhost';
> +
> +REVOKE ALL ON *.* FROM 'bup_some_priv'@'localhost';
> +
> +GRANT INSERT, UPDATE, DELETE, BACKUP, RESTORE, SELECT, CREATE, DROP
> + ON backup_test_trig.* TO 'bup_some_priv'@'localhost' WITH GRANT OPTION;
If one day a new privilege is added, which accidentally allows restore
of triggers, this would go undetected here. Won't it be more safe to
GRANT ALL and then REVOKE TRIGGER?
...
> +--echo #
> +--echo # conn_some_priv: Attempting restore. Should succeed.
> +--echo #
> +--replace_column 1 #
> +RESTORE FROM 'backup_test_trig.bak' OVERWRITE;
> +
> +disconnect conn_some_priv;
> +--echo #
> +--echo # Connect as root and cleanup.
> +--echo #
> +connect (conn_root,localhost,root,,);
> +
> +DROP DATABASE backup_test_trig;
> +
> +--echo #
> +--echo # Compare to original backup image file.
> +--echo #
> +
> +--replace_column 1 #
> +RESTORE FROM 'backup_test_orig.bak' OVERWRITE;
> +
> +--echo #
> +--echo # Show list of all objects in the database.
> +--echo #
> +SHOW FULL TABLES FROM backup_test;
> +SELECT event_name FROM INFORMATION_SCHEMA.EVENTS WHERE event_schema =
> 'backup_test';
> +SELECT routine_name FROM INFORMATION_SCHEMA.ROUTINES WHERE routine_schema =
> 'backup_test';
> +SELECT trigger_name FROM INFORMATION_SCHEMA.TRIGGERS WHERE trigger_schema =
> 'backup_test';
I do not understand, what this "compare to original" gives. After all we
don't show the objects after the other restore. So what do we compare
here, and to what?
...
> === modified file 'sql/backup/restore_info.h'
...
> + db= get_db(db_item->db->base.pos);
> + DBUG_EXECUTE_IF("ER_RESTORE_DB_ERROR", db= NULL;);
This is fine as long as there no other error injection point using the
same debug keyword. I use to include the function name in these keywords
to make them unique.
...
> + case BSTREAM_IT_DB:
> + {
> + result= check_access(thd, CREATE_ACL + DROP_ACL, name_str, 0, 1, 1, 0);
Not sure, if I mentioned this before. Could we make DROP_ACL dependent
on the OVERWITE keyword? If the database does not exist, RESTORE does
not need to DROP anything, does it?
...
> + /*
> + If the table is not found, revert to checking at the database
> + level.
> + */
> + if (!tbl)
> + {
> + result= check_access(thd, TRIGGER_ACL, db->name().ptr(), 0, 1, 1, 0);
I believe, I asked it above already. If so, no need to answer again.
What does this check give us? Can there be any circumstance, where
RESTORE can succeed if it includes a trigger for a non-existent table?
...
Regards
Ingo
--
Ingo Strüwing, Database Group
Sun Microsystems GmbH, Sonnenallee 1, D-85551 Kirchheim-Heimstetten
Geschäftsführer: Thomas Schröder, Wolfgang Engels, Wolf Frenkel
Vorsitzender des Aufsichtsrates: Martin Häring HRB München 161028
