Hi Chuck,
my approval stands. I do not require further changes. But I got the
impression that I might not have expressed my thoughts very well. Below,
I try to do better. Again, if I succeed in making you understand, what I
mean, and you still stick with your patch, so be it.
Charles Bell, 06.11.2009 14:58:
...
> Alas, but this was the wrong patch.
Sorry. I'll look at the new patches again, which you listed in your
email of Thu, 05 Nov 2009 14:52:06 -0500.
...
>> Chuck Bell, 02.11.2009 23:28:
...
>>> +# 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 put this test case in because with restore elevation off the
> prechecking code still runs. I want to show the patch through the code
> with prechecking off still generates an error -- that by turning both
> off the failure occurs elsewhere in the code and that deserves a special
> test case.
Yes, that's fine. But what I try to express: The result file does not
show the difference. In both cases RESTORE fails with
ER_BACKUP_CANT_RESTORE_DB. How do we tell that the "failure occurs
elsewhere in the code"?
If we show the existing objects after RESTORE, the difference becomes
obvious. I tried that, but there is no difference. This is because
RESTORE fails on the first database already. So the comment "(Restore
will fail in the middle)" is moot. Wouldn't is be great to set the
privileges so that it really fails in the middle? That is, after
dropping the database?
Grrm. Experimenting with the privileges I noticed a nice subtle
difference. Not only that you switch the system variables, but also the
privileges. Now I lost my understanding of the test strategy. I do now
have to admit that 2 and 3 test different things, but these are not,
what the comments say. Still, my complaint remains. The result file
doesn't show a difference between the tests. They do not prove that
something different happens inside the RESTORE code.
>
>> 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.
>
> All options are ON for the backup_security_var test. In this test I
> merely show the startup options control the same variable and that they
> can be reset and the code works as expected. This is also good to
> include if someone changes the startup options and/or tries to make the
> variables read only or some such. I think the test should stand as is.
My point is that you test the options only as long as you don't set the
variables. From the point of setting a variable, you stop to test the
option. In test 1 you "lose" backup_elevation since you set it to OFF,
which it is anyway. In test 2 you "lose" restore_elevation and in test 3
you lose the last one. So at least test 3 works only on variable settings.
The fact that setting the variable overrides the option is implicitly
tested in backup_security_var anyway.
>
> ...
>
>>> + 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.
>
> Elevation is turned off prior to this check. This check is performed per
> database. The fact that a user had RESTORE+SUPER for one of the
> databases does *not* mean he has RESTORE for all of the databases. Thus,
> with each check elevation must be turned off so that the RESTORE can be
> checked for each database.
>
>> 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.]
>
> No, that is not how the elevation works. Elevation works by turning on
> ALL privileges so that no matter what a statement will not fail for
> privilege issues.
Is it a universal law that "elevation works by turning on ALL
privileges"? What is wrong with my idea to turn on ALL *but* BACKUP_ACL,
RESTORE_ACL, and for BACKUP also SUPER_ACL?
If we elevate privileges in BACKUP, will any object require RESTORE_ACL
for being backed up?
If we elevate privileges in RESTORE, will any object require BACKUP_ACL
for being restored?
Excluding these two from the elevation should always work. And then we
won't need to rush the elevator up and down for every database. The one
privilege, we are interested in would always have its "real" value. The
other privileges may be elevated or not. We don't need them inside the
kernel.
SUPER_ACL is not that easy to handle. We don't check it (in the kernel)
during BACKUP, so it should be elevated for BACKUP. But we check it in
RESTORE, so it should not be elevated in RESTORE.
...
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
