Hi Chuck,
Status: Approved pending changes.
Required:
8) Indentation glitches (twice).
Request:
1) Improve revision comment. (like last patch)
2) Simplify revision comment.
3) Remove tests, if duplicate.
4) Narrow MTR suppressions. (twice)
5) Improve test case comment.
7) Suggestion for code improvement in ACL elevation/restore.
Options:
6) End-space.
Details:
Chuck Bell, 02.11.2009 23:28:
...
> 2889 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 turn off backup elevation, restore elevation, and restore
> prechecking by startup options and variables.
1) Same comment as on last patch.
>
> This patch implements startup options and variables to do the
> following:
>
> backup-elevation/backup_elevation ON = turn on backup elevation
> OFF = turn off backup elevation
> Note: Default is ON
>
> restore-elevation/restore_elevation ON = turn on restore elevation
> OFF = turn off restore elevation
> Note: Default is ON
>
> restore-precheck/restore_precheck ON = turn on restore precheck
> OFF = turn off restore precheck
> Note: Default is ON
2) IMHO mentioning the command line form with dash (-) doesn't add
information. I think, users are aware of the option to write the command
line options with dash instead of underline. And it is just an option.
Command line options can also be written with underline.
...
> === added file 'mysql-test/suite/backup/t/backup_security_options.test'
...
> +# Test Cases
> +# ----------
> +# 1. Ensure backup_elevation = OFF fails for not enough privileges.
> +# 2. Ensure restore_elevation = OFF fails for not enough privileges.
> +# 3. Ensure restore_precheck = OFF fails for not enough privileges
> +# (Restore will fail in the middle).
3) The difference in the tests for 2 and 3 are small. We don't really
see that RESTORE fails differently. I suggest to show the existing
objects after RESTORE. So one should see that all do still exist after
failed RESTORE in 2, but some are dropped in 3.
I understand that this test shall show that the command line options
work. However, during the test case we toggle the values using the
system variables. The tests themselves seem to be covered by
backup_security_var.test mostly. If this is correct, we could keep this
test case very short. Just show that all options are off, and start
backup_security_var.test with showing that all options are on.
> +#
> +
> +--source include/not_embedded.inc
> +
> +disable_query_log;
> +call mtr.add_suppression("Backup:");
> +call mtr.add_suppression("Restore:");
> +enable_query_log;
4) Hm. Déjà Vu?
...
> === added file 'mysql-test/suite/backup/t/backup_security_var.test'
...
> +# Test Cases
> +# ----------
> +# 1. Ensure backup_elevation = OFF fails for not enough privileges.
> +# 2. Show backup_elevation = OFF can succeed if privileges granted.
> +# 3. Show backup_elevation = ON can succeed if minimal privileges granted.
> +# 4. Ensure restore_elevation = OFF fails for not enough privileges.
> +# 5. Show restore_elevation = OFF can succeed if privileges granted.
> +# 6. Show restore_elevation = OFF and RESTORE + SUPER still fail.
> +# 7. Show restore_elevation = ON and RESTORE + SUPER still succeed.
5) I'd remove 'still' here.
> +# 8. Ensure restore_precheck = OFF fails for not enough privileges
> +# (Restore will fail in the middle).
> +# 9. Show restore_precheck = OFF can succeed if privileges granted.
> +# 10. Ensure restore_elevation = OFF and restore_precheck = OFF fails
> +# for not enough privileges.
> +# 11. Show restore_elevation = OFF and restore_precheck = OFF can succeed
> +# if privileges granted.
> +#
> +
> +--source include/not_embedded.inc
> +
> +disable_query_log;
> +call mtr.add_suppression("Backup:");
> +call mtr.add_suppression("Restore:");
> +enable_query_log;
4) You know.
...
> === modified file 'sql/backup/kernel.cc'
...
> @@ -2040,44 +2040,55 @@ int check_restore_privileges(struct st_b
> If user has RESTORE + SUPER, do not do object-level privilege checking.
>
> If the user does not have RESTORE, fail with an error.
> -
> +
6) Thank you, two blanks removed. :-) But one still left. :(
Ok, ok, ok, I know. I won't repeat these end-space complaints for now.
...
> + if (!check_access(info->m_thd, RESTORE_ACL, name_str, 0, 1, 1, 0)
> &&
> + (info->m_thd->security_ctx->master_access & SUPER_ACL)
> &&
> + info->m_skip_precheck)
7) Hm. Do I understand this correctly?: With restore_elevation we set
all global privileges, including RESTORE_ACL and SUPER_ACL. Now we need
to know, if the user has these privileges natively. Hence we restore the
original privileges and check for these two.
Great. That should work. But why the hack do we set RESTORE_ACL and
SUPER_ACL in the first place? Wouldn't it be nicer to have this in
set_all_global_privileges() (aka skip_grants_user())?:
master_access|= ~(BACKUP_ACL | RESTORE_ACL | SUPER_ACL);
Wouldn't this avoid the back and forth?
[Well, for BACKUP, we might want to elevate SUPER_ACL too. So we might
need two similar functions, or a parametrized one.]
> + {
> + /*
> + The base check for RESTORE_ACL + SUPER_ACL for this database is
> + satisfied. It is ok to elevate by turning off privilege checking.
> + */
8) Indentation (repeats).
...
> === modified file 'sql/mysqld.cc'
...
> @@ -6101,8 +6104,11 @@ struct my_option my_long_options[] =
> "Enable|disable backup progress log", (uchar**) &opt_backup_progress_log,
> (uchar**) &opt_backup_progress_log, 0, GET_BOOL, OPT_ARG, 1, 0, 0, 0, 0, 0},
> {"mysql-backup", OPT_MYSQL_BACKUP,
> - "Enable|disable MySQL Backup system", (uchar**) &opt_mysql_backup,
> - (uchar**) &opt_mysql_backup, 0, GET_BOOL, OPT_ARG, 0, 0, 0, 0, 0, 0},
> + "Enable|disable MySQL Backup system", (uchar**) &opt_mysql_backup,
> + (uchar**) &opt_mysql_backup, 0, GET_BOOL, OPT_ARG, 0, 0, 0, 0, 0, 0},
> + {"backup-elevation", OPT_BACKUP_ELEVATION,
> + "Enable|disable privilege elevaton for backup", (uchar**)
> &global_system_variables.backup_elevation,
> + (uchar**) &global_system_variables.backup_elevation, 0, GET_BOOL, OPT_ARG,
> 1, 0, 0, 0, 0, 0},
8) Please keep the indentation like the other options. (repeats)
...
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
