Hi Chuck,
Status: Approved pending changes.
Required.
9) Indentation (two places).
Requests:
1) Improve revision comment.
2) Improve file comment.
3) Fix typo (multiple places).
4) Explain excessive errors.
7) Is it *that* random, which error RESTORE will throw?
Options:
5) Exchange order of test paragraphs.
6) Don't disconnect/reconnect 'root' all the time.
8) We don't want to add end-space.
10) Function names.
Details:
Chuck Bell, 02.11.2009 22:57:
...
> 2888 Chuck Bell 2009-11-02
> BUG#44787 : Backup: Check privileges before executing BACKUP/RESTORE
>
> The backup system must be changed to meet the Server PT decision
> to elevate backup if BACKUP and restore if RESTORE + SUPER.
1) Usually we mean the operations when writing uppercase BACKUP or
RESTORE. I understand the above sentence so that the lowercase names are
the operations, while th uppercase names are the privileges. To avoid
ambiguity, can you please append _PRIV or _ACL to the privileges? Or
make it a full English sentence?
And/or be more verbose, so that someone, who is not familiar with all
our discussions, does also understand the purpose of the patch?
BTW "Server PT" is probably not well known to community developers.
>
> This patch implements the privilege elevation change for backup
> as well as privilege elevation for restore iff the user has both
> RESTORE and SUPER on all databases in the image. The restore
> will fall back to object-level privilege checking if this condition
> is not met.
>
> Note: This is patch 2 of 3. Patch 1 implements privilege checking,
> patch 3 implements the options to skip precheck and turn
> elevation off.
...
> @ sql/sql_class.h
> Added methods to preserve privilege access.
2) "Preserve privilege access"? Is the access to privileges endangered
by the patch somehow? Do you mean methods to "save, set, and restore
privileges"? IMHO it can sometimes make things clearer if using exact
wording.
...
> === modified file 'mysql-test/suite/backup/r/backup_restore_security.result'
...
> +# Test Case 8 : Show that user can have only TRIGGER + SUPER and
> +# perform an elevated restore.
3) TRIGGER + SUPER ? -> RESTORE + SUPER ?
...
> +# Test Case 9 : Show that users who have RESTORE + SUPER for db1 but not
> +# for db2 cannot restore the image.
> +#
...
> +RESTORE FROM 'backup_test_orig_alt1.bak' OVERWRITE;
> +ERROR HY000: Insufficient privileges. You must have the RESTORE privilege to restore
> database 'backup_test_alt'.
> +SHOW ERRORS;
> +Level Code Message
> +Error 1794 Insufficient privileges. You must have the RESTORE privilege to restore
> database 'backup_test_alt'.
> +Error 1798 Insufficient privileges. You do not have privileges to restore the object
> 't1' from this backup image.
4) Does this refer to backup_test.t1? Isn't the restore aborted on
database level for 'backup_test_alt' already? So backup_test_alt.t1
shouldn't even be tried? Anyway, don't we abort at the first missing
privilege?
...
> === modified file 'mysql-test/suite/backup/t/backup_restore_security.test'
...
> @@ -12,6 +12,10 @@
> # 6. 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.
> # 7. Show that having TRIGGER on a specific table permits restore.
> +# 8. Show that user can have only TRIGGER + SUPER and perform an
> +# elevated restore.
3) TRIGGER + SUPER ? -> RESTORE + SUPER ?
...
> --echo #
> +--echo # Test Case 8 : Show that user can have only TRIGGER + SUPER and
> +--echo # perform an elevated restore.
3) TRIGGER + SUPER ? -> RESTORE + SUPER ?
...
> +disconnect conn_some_priv;
> +--echo #
> +--echo # Change privileges for elevated restore test case.
> +--echo #
> +connect (conn_root,localhost,root,,);
> +
> +--echo #
> +--echo # Test Case 9 : Show that users who have RESTORE + SUPER for db1 but not
> +--echo # for db2 cannot restore the image.
> +--echo #
5) This looks like the main heading for a compound test case.
The 'connect' before it, together with its comment ("Change
privileges...") seems to belong to the test case. So I would find it
more logical to exchange the order of these two paragraphs.
> +
> +DROP USER 'bup_some_priv'@'localhost';
> +
> +FLUSH PRIVILEGES;
> +
> +CREATE USER 'bup_some_priv'@'localhost';
> +
> +REVOKE ALL ON *.* FROM 'bup_some_priv'@'localhost';
> +GRANT RESTORE ON backup_test.* TO 'bup_some_priv'@'localhost';
> +GRANT SUPER ON *.* TO 'bup_some_priv'@'localhost';
> +
> +FLUSH PRIVILEGES;
> +
> +disconnect conn_root;
6) For future tests like this one, I suggest to consider, not to
disconnect and reconnect 'root' every now and then. Just use
--connection to switch back to it. Even better might be, just to use the
default connection, which is 'root' anyway.
The advantage is not only a negligible performance win, but also the
recognition value of the root connection in debug traces. For every new
connect, a new thread number is selected. When following a trace one
easily confuses the root connection with the non-privileged test
connection. Having the root connection keeping its thread number, this
works much better.
...
> === modified file 'mysql-test/suite/backup/t/backup_security.test'
...
> @@ -188,7 +188,7 @@ SHOW TABLES FROM backup_test;
> --echo # catalog errors.
> --echo #
> --replace_column 1 #
> ---error ER_BACKUP_CANT_RESTORE_DB, ER_BACKUP_CANT_RESTORE_TABLE,
> ER_BACKUP_CANT_RESTORE_VIEW, ER_BACKUP_CANT_RESTORE_SROUT, ER_BACKUP_CANT_RESTORE_EVENT,
> ER_BACKUP_CANT_RESTORE_TRIGGER
> +--error ER_BACKUP_CANT_RESTORE_DB, ER_BACKUP_CANT_RESTORE_TABLE,
> ER_BACKUP_CANT_RESTORE_VIEW, ER_BACKUP_CANT_RESTORE_SROUT, ER_BACKUP_CANT_RESTORE_EVENT,
> ER_BACKUP_CANT_RESTORE_TRIGGER, ER_RESTORE_ACCESS_OBJS_INCOMPLETE
> eval RESTORE FROM 'bup_user1.bak' OVERWRITE;
7) Is it *that* random, which error RESTORE will throw?
...
> === modified file 'sql/backup/backup_info.cc'
...
> @@ -665,21 +665,31 @@ backup::Image_info::Db* Backup_info::add
> /*
> Check privileges for this database. User must have BACKUP
> privilege in order to execute a backup.
> +
> + We must turn privilege checking back on first.
> */
> + m_thd->security_ctx->reset_grant_info();
> +
> DEBUG_SYNC(m_thd, "before_backup_privileges");
> if (check_access(m_thd, BACKUP_ACL, name->ptr(), 0, 1, 1, 0))
> {
> m_log.report_error(ER_BACKUP_ACCESS_DENIED_ERROR, name->ptr());
> return NULL;
> - }
> + }
8) We don't want to add end-space. (repeats a couple of times)
...
> === modified file 'sql/backup/kernel.cc'
...
> @@ -639,6 +639,11 @@ int Backup_restore_ctx::prepare_path(::S
>
> int Backup_restore_ctx::prepare(::String *backupdir, LEX_STRING location)
> {
> + /*
> + Save privilege information for restoring later.
> + */
9) Indentation. (repeats for many multi-line comments.)
...
> @@ -1961,27 +1971,27 @@ int bcat_close(st_bstream_image_header *
>
>
> /**
> - Check access to a database-level object.
> + Check access to a database-level object.
9) Broke valid indentation. (whole function comment.) (repeats for next
function comment.)
...
> === modified file 'sql/sql_class.cc'
...
> +// Turns off grants but keeps user context.
> +void Security_context::skip_grants_user()
> +{
> + master_access= ~NO_ACCESS;
> +}
> +
> +// Saves current privilege settings.
> +void Security_context::save_grant_info()
> +{
> + /* save the values for restoring later with reset_grant_info() */
> + m_saved_master_access= master_access;
> +}
> +
> +// Turns grants back on.
> +void Security_context::reset_grant_info()
> +{
> + master_access= m_saved_master_access;
> +}
10) Were these names selected to make my life more difficult? I doubt
that I can remember these terms:
skip: no, do not skip something here, but set all privileges.
reset: no do not reset to default values, but restore to what was saved.
Please correct me if I'm wrong, but aren't save and restore used as
pairs frequently in technical documents?
I would like these functions to be called something like:
set_all_global_privileges, save_global_privileges,
restore_global_privileges.
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
